| Message ID | 20231125111605.47536-1-rogerq@kernel.org |
|---|---|
| State | Superseded |
| Delegated to: | Dario Binacchi |
| Headers | show |
| Series | mtd: nand: omap_gpmc: Fix NAND in SPL for AM335x | expand |
Hi Roger On Sat, Nov 25, 2023 at 12:16 PM Roger Quadros <rogerq@kernel.org> wrote: > > AM335x uses a special driver "am335x_spl_bch.c" as SPL > NAND loader. This driver expects 1 sector at a time ECC > and doesn't work well with multi-sector ECC that was implemented in > commit 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based correction") > > Switch back to 1 sector at a time read/ECC. > > Fixes: 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based correction") > Signed-off-by: Roger Quadros <rogerq@kernel.org> > --- > drivers/mtd/nand/raw/omap_gpmc.c | 95 ++++++++++---------------------- > 1 file changed, 29 insertions(+), 66 deletions(-) > > diff --git a/drivers/mtd/nand/raw/omap_gpmc.c b/drivers/mtd/nand/raw/omap_gpmc.c > index 1a5ed0de31..2d2d2c2b6d 100644 > --- a/drivers/mtd/nand/raw/omap_gpmc.c > +++ b/drivers/mtd/nand/raw/omap_gpmc.c > @@ -293,7 +293,7 @@ static void __maybe_unused omap_enable_hwecc_bch(struct mtd_info *mtd, > break; > case OMAP_ECC_BCH8_CODE_HW: > bch_type = 1; > - nsectors = chip->ecc.steps; > + nsectors = 1; > if (mode == NAND_ECC_READ) { > wr_mode = BCH_WRAPMODE_1; > ecc_size0 = BCH8R_ECC_SIZE0; > @@ -306,7 +306,7 @@ static void __maybe_unused omap_enable_hwecc_bch(struct mtd_info *mtd, > break; > case OMAP_ECC_BCH16_CODE_HW: > bch_type = 0x2; > - nsectors = chip->ecc.steps; > + nsectors = 1; > if (mode == NAND_ECC_READ) { > wr_mode = 0x01; > ecc_size0 = 52; /* ECC bits in nibbles per sector */ > @@ -345,17 +345,16 @@ static void __maybe_unused omap_enable_hwecc_bch(struct mtd_info *mtd, > } > > /** > - * _omap_calculate_ecc_bch - Generate BCH ECC bytes for one sector > + * omap_calculate_ecc_bch - Generate BCH ECC bytes for one sector > * @mtd: MTD device structure > * @dat: The pointer to data on which ecc is computed > * @ecc_code: The ecc_code buffer > - * @sector: The sector number (for a multi sector page) > * > * Support calculating of BCH4/8/16 ECC vectors for one sector > * within a page. Sector number is in @sector. > */ > -static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, > - u8 *ecc_code, int sector) > +static int omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, > + u8 *ecc_code) > { > struct nand_chip *chip = mtd_to_nand(mtd); > struct omap_nand_info *info = nand_get_controller_data(chip); > @@ -368,7 +367,7 @@ static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, > case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW: > #endif > case OMAP_ECC_BCH8_CODE_HW: > - ptr = &gpmc_cfg->bch_result_0_3[sector].bch_result_x[3]; > + ptr = &gpmc_cfg->bch_result_0_3[0].bch_result_x[3]; > val = readl(ptr); > ecc_code[i++] = (val >> 0) & 0xFF; > ptr--; > @@ -383,21 +382,21 @@ static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, > > break; > case OMAP_ECC_BCH16_CODE_HW: > - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[2]); > + val = readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[2]); > ecc_code[i++] = (val >> 8) & 0xFF; > ecc_code[i++] = (val >> 0) & 0xFF; > - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[1]); > + val = readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[1]); > ecc_code[i++] = (val >> 24) & 0xFF; > ecc_code[i++] = (val >> 16) & 0xFF; > ecc_code[i++] = (val >> 8) & 0xFF; > ecc_code[i++] = (val >> 0) & 0xFF; > - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[0]); > + val = readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[0]); > ecc_code[i++] = (val >> 24) & 0xFF; > ecc_code[i++] = (val >> 16) & 0xFF; > ecc_code[i++] = (val >> 8) & 0xFF; > ecc_code[i++] = (val >> 0) & 0xFF; > for (j = 3; j >= 0; j--) { > - val = readl(&gpmc_cfg->bch_result_0_3[sector].bch_result_x[j] > + val = readl(&gpmc_cfg->bch_result_0_3[0].bch_result_x[j] > ); > ecc_code[i++] = (val >> 24) & 0xFF; > ecc_code[i++] = (val >> 16) & 0xFF; > @@ -431,22 +430,6 @@ static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, > return 0; > } > > -/** > - * omap_calculate_ecc_bch - ECC generator for 1 sector > - * @mtd: MTD device structure > - * @dat: The pointer to data on which ecc is computed > - * @ecc_code: The ecc_code buffer > - * > - * Support calculating of BCH4/8/16 ECC vectors for one sector. This is used > - * when SW based correction is required as ECC is required for one sector > - * at a time. > - */ > -static int __maybe_unused omap_calculate_ecc_bch(struct mtd_info *mtd, > - const u_char *dat, u_char *ecc_calc) > -{ > - return _omap_calculate_ecc_bch(mtd, dat, ecc_calc, 0); > -} > - > static inline void omap_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len) > { > struct nand_chip *chip = mtd_to_nand(mtd); > @@ -572,34 +555,6 @@ static void omap_nand_read_prefetch(struct mtd_info *mtd, uint8_t *buf, int len) > > #ifdef CONFIG_NAND_OMAP_ELM > > -/** > - * omap_calculate_ecc_bch_multi - Generate ECC for multiple sectors > - * @mtd: MTD device structure > - * @dat: The pointer to data on which ecc is computed > - * @ecc_code: The ecc_code buffer > - * > - * Support calculating of BCH4/8/16 ecc vectors for the entire page in one go. > - */ > -static int omap_calculate_ecc_bch_multi(struct mtd_info *mtd, > - const u_char *dat, u_char *ecc_calc) > -{ > - struct nand_chip *chip = mtd_to_nand(mtd); > - int eccbytes = chip->ecc.bytes; > - unsigned long nsectors; > - int i, ret; > - > - nsectors = ((readl(&gpmc_cfg->ecc_config) >> 4) & 0x7) + 1; > - for (i = 0; i < nsectors; i++) { > - ret = _omap_calculate_ecc_bch(mtd, dat, ecc_calc, i); > - if (ret) > - return ret; > - > - ecc_calc += eccbytes; > - } > - > - return 0; > -} > - > /* > * omap_reverse_list - re-orders list elements in reverse order [internal] > * @list: pointer to start of list > @@ -752,7 +707,6 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip, > { > int i, eccsize = chip->ecc.size; > int eccbytes = chip->ecc.bytes; > - int ecctotal = chip->ecc.total; > int eccsteps = chip->ecc.steps; > uint8_t *p = buf; > uint8_t *ecc_calc = chip->buffers->ecccalc; > @@ -760,24 +714,30 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip, > uint32_t *eccpos = chip->ecc.layout->eccpos; > uint8_t *oob = chip->oob_poi; > uint32_t oob_pos; > + u32 data_pos = 0; > > /* oob area start */ > oob_pos = (eccsize * eccsteps) + chip->ecc.layout->eccpos[0]; > oob += chip->ecc.layout->eccpos[0]; > > - /* Enable ECC engine */ > - chip->ecc.hwctl(mtd, NAND_ECC_READ); > + for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize, > + oob += eccbytes) { > + /* Enable ECC engine */ > + chip->ecc.hwctl(mtd, NAND_ECC_READ); > > - /* read entire page */ > - chip->cmdfunc(mtd, NAND_CMD_RNDOUT, 0, -1); > - chip->read_buf(mtd, buf, mtd->writesize); > + /* read data */ > + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, data_pos, -1); > + chip->read_buf(mtd, p, eccsize); > > - /* read all ecc bytes from oob area */ > - chip->cmdfunc(mtd, NAND_CMD_RNDOUT, oob_pos, -1); > - chip->read_buf(mtd, oob, ecctotal); > + /* read respective ecc from oob area */ > + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, oob_pos, -1); > + chip->read_buf(mtd, oob, eccbytes); > + /* read syndrome */ > + chip->ecc.calculate(mtd, p, &ecc_calc[i]); > > - /* Calculate ecc bytes */ > - omap_calculate_ecc_bch_multi(mtd, buf, ecc_calc); > + data_pos += eccsize; > + oob_pos += eccbytes; > + } > > for (i = 0; i < chip->ecc.total; i++) > ecc_code[i] = chip->oob_poi[eccpos[i]]; > @@ -945,6 +905,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, > nand->ecc.hwctl = omap_enable_hwecc_bch; > nand->ecc.correct = omap_correct_data_bch_sw; > nand->ecc.calculate = omap_calculate_ecc_bch; > + nand->ecc.steps = eccsteps; > /* define ecc-layout */ > ecclayout->eccbytes = nand->ecc.bytes * eccsteps; > ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH; > @@ -987,6 +948,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, > nand->ecc.correct = omap_correct_data_bch; > nand->ecc.calculate = omap_calculate_ecc_bch; > nand->ecc.read_page = omap_read_page_bch; > + nand->ecc.steps = eccsteps; > /* define ecc-layout */ > ecclayout->eccbytes = nand->ecc.bytes * eccsteps; > for (i = 0; i < ecclayout->eccbytes; i++) > @@ -1020,6 +982,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, > nand->ecc.correct = omap_correct_data_bch; > nand->ecc.calculate = omap_calculate_ecc_bch; > nand->ecc.read_page = omap_read_page_bch; > + nand->ecc.steps = eccsteps; > /* define ecc-layout */ > ecclayout->eccbytes = nand->ecc.bytes * eccsteps; > for (i = 0; i < ecclayout->eccbytes; i++) > > base-commit: 9e53e45292ee2f1d9d2ccc59914b161bef9b10d7 > -- > 2.34.1 > Let's wait on some tested-by Michael
On Sat, Nov 25, 2023 at 01:16:05PM +0200, Roger Quadros wrote: > AM335x uses a special driver "am335x_spl_bch.c" as SPL > NAND loader. This driver expects 1 sector at a time ECC > and doesn't work well with multi-sector ECC that was implemented in > commit 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based correction") > > Switch back to 1 sector at a time read/ECC. > > Fixes: 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based correction") > Signed-off-by: Roger Quadros <rogerq@kernel.org> > --- > drivers/mtd/nand/raw/omap_gpmc.c | 95 ++++++++++---------------------- > 1 file changed, 29 insertions(+), 66 deletions(-) I'm glad to see this fixed. My question is, can we abstract this slightly as I assume there's a performance hit on the newer SoCs that support more than one sector at a time for ECC and I assume it's just am335x and related that don't support the feature. Thanks.
Hello Roger, On 25.11.23 12:16, Roger Quadros wrote: > AM335x uses a special driver "am335x_spl_bch.c" as SPL > NAND loader. This driver expects 1 sector at a time ECC > and doesn't work well with multi-sector ECC that was implemented in > commit 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based correction") > > Switch back to 1 sector at a time read/ECC. > > Fixes: 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based correction") > Signed-off-by: Roger Quadros <rogerq@kernel.org> > --- > drivers/mtd/nand/raw/omap_gpmc.c | 95 ++++++++++---------------------- > 1 file changed, 29 insertions(+), 66 deletions(-) [...] > > base-commit: 9e53e45292ee2f1d9d2ccc59914b161bef9b10d7 Based on this commit and with the (rebased) patchset from Enrico: https://lists.denx.de/pipermail/u-boot/2023-November/536793.html I can confirm, that the draco thuban board now boots again from NAND. Tested-by: Heiko Schocher <hs@denx.de> Thanks! bye, Heiko
Hi, Works on my draco thuban AM335x based boards booting from NAND with ECC BCH8 code. Tested-by: Enrico Leto <enrico.leto@siemens.com> Thanks > -----Original Message----- > From: Michael Nazzareno Trimarchi <michael@amarulasolutions.com> > Sent: Saturday, November 25, 2023 2:07 PM > To: Roger Quadros <rogerq@kernel.org> > Cc: dario.binacchi@amarulasolutions.com; Schocher, Heiko (EXT) (DENX > Software Engineering GmbH) <hs@denx.de>; Leto, Enrico (SI BP R&D ZG FW > CCP) <enrico.leto@siemens.com>; trini@konsulko.com; praneeth@ti.com; > nm@ti.com; vigneshr@ti.com; u-boot@lists.denx.de > Subject: Re: [PATCH] mtd: nand: omap_gpmc: Fix NAND in SPL for AM335x > > Hi Roger > > On Sat, Nov 25, 2023 at 12:16 PM Roger Quadros <rogerq@kernel.org> > wrote: > > > > AM335x uses a special driver "am335x_spl_bch.c" as SPL NAND loader. > > This driver expects 1 sector at a time ECC and doesn't work well with > > multi-sector ECC that was implemented in commit 04fcd2587321 ("mtd: > > rawnand: omap_gpmc: Fix BCH6/16 HW based correction") > > > > Switch back to 1 sector at a time read/ECC. > > > > Fixes: 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based > > correction") > > Signed-off-by: Roger Quadros <rogerq@kernel.org> > > --- > > drivers/mtd/nand/raw/omap_gpmc.c | 95 > > ++++++++++---------------------- > > 1 file changed, 29 insertions(+), 66 deletions(-) > > > > diff --git a/drivers/mtd/nand/raw/omap_gpmc.c > > b/drivers/mtd/nand/raw/omap_gpmc.c > > index 1a5ed0de31..2d2d2c2b6d 100644 > > --- a/drivers/mtd/nand/raw/omap_gpmc.c > > +++ b/drivers/mtd/nand/raw/omap_gpmc.c > > @@ -293,7 +293,7 @@ static void __maybe_unused > omap_enable_hwecc_bch(struct mtd_info *mtd, > > break; > > case OMAP_ECC_BCH8_CODE_HW: > > bch_type = 1; > > - nsectors = chip->ecc.steps; > > + nsectors = 1; > > if (mode == NAND_ECC_READ) { > > wr_mode = BCH_WRAPMODE_1; > > ecc_size0 = BCH8R_ECC_SIZE0; @@ -306,7 +306,7 > > @@ static void __maybe_unused omap_enable_hwecc_bch(struct mtd_info > *mtd, > > break; > > case OMAP_ECC_BCH16_CODE_HW: > > bch_type = 0x2; > > - nsectors = chip->ecc.steps; > > + nsectors = 1; > > if (mode == NAND_ECC_READ) { > > wr_mode = 0x01; > > ecc_size0 = 52; /* ECC bits in nibbles per > > sector */ @@ -345,17 +345,16 @@ static void __maybe_unused > > omap_enable_hwecc_bch(struct mtd_info *mtd, } > > > > /** > > - * _omap_calculate_ecc_bch - Generate BCH ECC bytes for one sector > > + * omap_calculate_ecc_bch - Generate BCH ECC bytes for one sector > > * @mtd: MTD device structure > > * @dat: The pointer to data on which ecc is computed > > * @ecc_code: The ecc_code buffer > > - * @sector: The sector number (for a multi sector page) > > * > > * Support calculating of BCH4/8/16 ECC vectors for one sector > > * within a page. Sector number is in @sector. > > */ > > -static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, > > - u8 *ecc_code, int sector) > > +static int omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, > > + u8 *ecc_code) > > { > > struct nand_chip *chip = mtd_to_nand(mtd); > > struct omap_nand_info *info = nand_get_controller_data(chip); > > @@ -368,7 +367,7 @@ static int _omap_calculate_ecc_bch(struct mtd_info > *mtd, const u8 *dat, > > case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW: > > #endif > > case OMAP_ECC_BCH8_CODE_HW: > > - ptr = &gpmc_cfg->bch_result_0_3[sector].bch_result_x[3]; > > + ptr = &gpmc_cfg->bch_result_0_3[0].bch_result_x[3]; > > val = readl(ptr); > > ecc_code[i++] = (val >> 0) & 0xFF; > > ptr--; > > @@ -383,21 +382,21 @@ static int _omap_calculate_ecc_bch(struct > > mtd_info *mtd, const u8 *dat, > > > > break; > > case OMAP_ECC_BCH16_CODE_HW: > > - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[2]); > > + val = > > + readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[2]); > > ecc_code[i++] = (val >> 8) & 0xFF; > > ecc_code[i++] = (val >> 0) & 0xFF; > > - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[1]); > > + val = > > + readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[1]); > > ecc_code[i++] = (val >> 24) & 0xFF; > > ecc_code[i++] = (val >> 16) & 0xFF; > > ecc_code[i++] = (val >> 8) & 0xFF; > > ecc_code[i++] = (val >> 0) & 0xFF; > > - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[0]); > > + val = > > + readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[0]); > > ecc_code[i++] = (val >> 24) & 0xFF; > > ecc_code[i++] = (val >> 16) & 0xFF; > > ecc_code[i++] = (val >> 8) & 0xFF; > > ecc_code[i++] = (val >> 0) & 0xFF; > > for (j = 3; j >= 0; j--) { > > - val = readl(&gpmc_cfg->bch_result_0_3[sector].bch_result_x[j] > > + val = > > + readl(&gpmc_cfg->bch_result_0_3[0].bch_result_x[j] > > ); > > ecc_code[i++] = (val >> 24) & 0xFF; > > ecc_code[i++] = (val >> 16) & 0xFF; @@ -431,22 > > +430,6 @@ static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const > u8 *dat, > > return 0; > > } > > > > -/** > > - * omap_calculate_ecc_bch - ECC generator for 1 sector > > - * @mtd: MTD device structure > > - * @dat: The pointer to data on which ecc is computed > > - * @ecc_code: The ecc_code buffer > > - * > > - * Support calculating of BCH4/8/16 ECC vectors for one sector. This > > is used > > - * when SW based correction is required as ECC is required for one > > sector > > - * at a time. > > - */ > > -static int __maybe_unused omap_calculate_ecc_bch(struct mtd_info *mtd, > > - const u_char *dat, u_char *ecc_calc) > > -{ > > - return _omap_calculate_ecc_bch(mtd, dat, ecc_calc, 0); > > -} > > - > > static inline void omap_nand_read_buf(struct mtd_info *mtd, uint8_t > > *buf, int len) { > > struct nand_chip *chip = mtd_to_nand(mtd); @@ -572,34 +555,6 > > @@ static void omap_nand_read_prefetch(struct mtd_info *mtd, uint8_t > > *buf, int len) > > > > #ifdef CONFIG_NAND_OMAP_ELM > > > > -/** > > - * omap_calculate_ecc_bch_multi - Generate ECC for multiple sectors > > - * @mtd: MTD device structure > > - * @dat: The pointer to data on which ecc is computed > > - * @ecc_code: The ecc_code buffer > > - * > > - * Support calculating of BCH4/8/16 ecc vectors for the entire page in one > go. > > - */ > > -static int omap_calculate_ecc_bch_multi(struct mtd_info *mtd, > > - const u_char *dat, u_char *ecc_calc) > > -{ > > - struct nand_chip *chip = mtd_to_nand(mtd); > > - int eccbytes = chip->ecc.bytes; > > - unsigned long nsectors; > > - int i, ret; > > - > > - nsectors = ((readl(&gpmc_cfg->ecc_config) >> 4) & 0x7) + 1; > > - for (i = 0; i < nsectors; i++) { > > - ret = _omap_calculate_ecc_bch(mtd, dat, ecc_calc, i); > > - if (ret) > > - return ret; > > - > > - ecc_calc += eccbytes; > > - } > > - > > - return 0; > > -} > > - > > /* > > * omap_reverse_list - re-orders list elements in reverse order [internal] > > * @list: pointer to start of list > > @@ -752,7 +707,6 @@ static int omap_read_page_bch(struct mtd_info > > *mtd, struct nand_chip *chip, { > > int i, eccsize = chip->ecc.size; > > int eccbytes = chip->ecc.bytes; > > - int ecctotal = chip->ecc.total; > > int eccsteps = chip->ecc.steps; > > uint8_t *p = buf; > > uint8_t *ecc_calc = chip->buffers->ecccalc; @@ -760,24 +714,30 > > @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip > *chip, > > uint32_t *eccpos = chip->ecc.layout->eccpos; > > uint8_t *oob = chip->oob_poi; > > uint32_t oob_pos; > > + u32 data_pos = 0; > > > > /* oob area start */ > > oob_pos = (eccsize * eccsteps) + chip->ecc.layout->eccpos[0]; > > oob += chip->ecc.layout->eccpos[0]; > > > > - /* Enable ECC engine */ > > - chip->ecc.hwctl(mtd, NAND_ECC_READ); > > + for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize, > > + oob += eccbytes) { > > + /* Enable ECC engine */ > > + chip->ecc.hwctl(mtd, NAND_ECC_READ); > > > > - /* read entire page */ > > - chip->cmdfunc(mtd, NAND_CMD_RNDOUT, 0, -1); > > - chip->read_buf(mtd, buf, mtd->writesize); > > + /* read data */ > > + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, data_pos, -1); > > + chip->read_buf(mtd, p, eccsize); > > > > - /* read all ecc bytes from oob area */ > > - chip->cmdfunc(mtd, NAND_CMD_RNDOUT, oob_pos, -1); > > - chip->read_buf(mtd, oob, ecctotal); > > + /* read respective ecc from oob area */ > > + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, oob_pos, -1); > > + chip->read_buf(mtd, oob, eccbytes); > > + /* read syndrome */ > > + chip->ecc.calculate(mtd, p, &ecc_calc[i]); > > > > - /* Calculate ecc bytes */ > > - omap_calculate_ecc_bch_multi(mtd, buf, ecc_calc); > > + data_pos += eccsize; > > + oob_pos += eccbytes; > > + } > > > > for (i = 0; i < chip->ecc.total; i++) > > ecc_code[i] = chip->oob_poi[eccpos[i]]; @@ -945,6 > > +905,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, > > nand->ecc.hwctl = omap_enable_hwecc_bch; > > nand->ecc.correct = omap_correct_data_bch_sw; > > nand->ecc.calculate = omap_calculate_ecc_bch; > > + nand->ecc.steps = eccsteps; > > /* define ecc-layout */ > > ecclayout->eccbytes = nand->ecc.bytes * eccsteps; > > ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH; > > @@ -987,6 +948,7 @@ static int omap_select_ecc_scheme(struct > nand_chip *nand, > > nand->ecc.correct = omap_correct_data_bch; > > nand->ecc.calculate = omap_calculate_ecc_bch; > > nand->ecc.read_page = omap_read_page_bch; > > + nand->ecc.steps = eccsteps; > > /* define ecc-layout */ > > ecclayout->eccbytes = nand->ecc.bytes * eccsteps; > > for (i = 0; i < ecclayout->eccbytes; i++) @@ -1020,6 > > +982,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, > > nand->ecc.correct = omap_correct_data_bch; > > nand->ecc.calculate = omap_calculate_ecc_bch; > > nand->ecc.read_page = omap_read_page_bch; > > + nand->ecc.steps = eccsteps; > > /* define ecc-layout */ > > ecclayout->eccbytes = nand->ecc.bytes * eccsteps; > > for (i = 0; i < ecclayout->eccbytes; i++) > > > > base-commit: 9e53e45292ee2f1d9d2ccc59914b161bef9b10d7 > > -- > > 2.34.1 > > > > Let's wait on some tested-by > > Michael
Hi dario On Mon, Nov 27, 2023 at 3:00 PM Leto, Enrico <enrico.leto@siemens.com> wrote: > > Hi, > > Works on my draco thuban AM335x based boards booting from NAND with ECC BCH8 code. > > Tested-by: Enrico Leto <enrico.leto@siemens.com> > > Thanks > > > > -----Original Message----- > > From: Michael Nazzareno Trimarchi <michael@amarulasolutions.com> > > Sent: Saturday, November 25, 2023 2:07 PM > > To: Roger Quadros <rogerq@kernel.org> > > Cc: dario.binacchi@amarulasolutions.com; Schocher, Heiko (EXT) (DENX > > Software Engineering GmbH) <hs@denx.de>; Leto, Enrico (SI BP R&D ZG FW > > CCP) <enrico.leto@siemens.com>; trini@konsulko.com; praneeth@ti.com; > > nm@ti.com; vigneshr@ti.com; u-boot@lists.denx.de > > Subject: Re: [PATCH] mtd: nand: omap_gpmc: Fix NAND in SPL for AM335x > > > > Hi Roger > > > > On Sat, Nov 25, 2023 at 12:16 PM Roger Quadros <rogerq@kernel.org> > > wrote: > > > > > > AM335x uses a special driver "am335x_spl_bch.c" as SPL NAND loader. > > > This driver expects 1 sector at a time ECC and doesn't work well with > > > multi-sector ECC that was implemented in commit 04fcd2587321 ("mtd: > > > rawnand: omap_gpmc: Fix BCH6/16 HW based correction") > > > > > > Switch back to 1 sector at a time read/ECC. > > > > > > Fixes: 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based > > > correction") > > > Signed-off-by: Roger Quadros <rogerq@kernel.org> > > > --- > > > drivers/mtd/nand/raw/omap_gpmc.c | 95 > > > ++++++++++---------------------- > > > 1 file changed, 29 insertions(+), 66 deletions(-) > > > > > > diff --git a/drivers/mtd/nand/raw/omap_gpmc.c > > > b/drivers/mtd/nand/raw/omap_gpmc.c > > > index 1a5ed0de31..2d2d2c2b6d 100644 > > > --- a/drivers/mtd/nand/raw/omap_gpmc.c > > > +++ b/drivers/mtd/nand/raw/omap_gpmc.c > > > @@ -293,7 +293,7 @@ static void __maybe_unused > > omap_enable_hwecc_bch(struct mtd_info *mtd, > > > break; > > > case OMAP_ECC_BCH8_CODE_HW: > > > bch_type = 1; > > > - nsectors = chip->ecc.steps; > > > + nsectors = 1; > > > if (mode == NAND_ECC_READ) { > > > wr_mode = BCH_WRAPMODE_1; > > > ecc_size0 = BCH8R_ECC_SIZE0; @@ -306,7 +306,7 > > > @@ static void __maybe_unused omap_enable_hwecc_bch(struct mtd_info > > *mtd, > > > break; > > > case OMAP_ECC_BCH16_CODE_HW: > > > bch_type = 0x2; > > > - nsectors = chip->ecc.steps; > > > + nsectors = 1; > > > if (mode == NAND_ECC_READ) { > > > wr_mode = 0x01; > > > ecc_size0 = 52; /* ECC bits in nibbles per > > > sector */ @@ -345,17 +345,16 @@ static void __maybe_unused > > > omap_enable_hwecc_bch(struct mtd_info *mtd, } > > > > > > /** > > > - * _omap_calculate_ecc_bch - Generate BCH ECC bytes for one sector > > > + * omap_calculate_ecc_bch - Generate BCH ECC bytes for one sector > > > * @mtd: MTD device structure > > > * @dat: The pointer to data on which ecc is computed > > > * @ecc_code: The ecc_code buffer > > > - * @sector: The sector number (for a multi sector page) > > > * > > > * Support calculating of BCH4/8/16 ECC vectors for one sector > > > * within a page. Sector number is in @sector. > > > */ > > > -static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, > > > - u8 *ecc_code, int sector) > > > +static int omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, > > > + u8 *ecc_code) > > > { > > > struct nand_chip *chip = mtd_to_nand(mtd); > > > struct omap_nand_info *info = nand_get_controller_data(chip); > > > @@ -368,7 +367,7 @@ static int _omap_calculate_ecc_bch(struct mtd_info > > *mtd, const u8 *dat, > > > case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW: > > > #endif > > > case OMAP_ECC_BCH8_CODE_HW: > > > - ptr = &gpmc_cfg->bch_result_0_3[sector].bch_result_x[3]; > > > + ptr = &gpmc_cfg->bch_result_0_3[0].bch_result_x[3]; > > > val = readl(ptr); > > > ecc_code[i++] = (val >> 0) & 0xFF; > > > ptr--; > > > @@ -383,21 +382,21 @@ static int _omap_calculate_ecc_bch(struct > > > mtd_info *mtd, const u8 *dat, > > > > > > break; > > > case OMAP_ECC_BCH16_CODE_HW: > > > - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[2]); > > > + val = > > > + readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[2]); > > > ecc_code[i++] = (val >> 8) & 0xFF; > > > ecc_code[i++] = (val >> 0) & 0xFF; > > > - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[1]); > > > + val = > > > + readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[1]); > > > ecc_code[i++] = (val >> 24) & 0xFF; > > > ecc_code[i++] = (val >> 16) & 0xFF; > > > ecc_code[i++] = (val >> 8) & 0xFF; > > > ecc_code[i++] = (val >> 0) & 0xFF; > > > - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[0]); > > > + val = > > > + readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[0]); > > > ecc_code[i++] = (val >> 24) & 0xFF; > > > ecc_code[i++] = (val >> 16) & 0xFF; > > > ecc_code[i++] = (val >> 8) & 0xFF; > > > ecc_code[i++] = (val >> 0) & 0xFF; > > > for (j = 3; j >= 0; j--) { > > > - val = readl(&gpmc_cfg->bch_result_0_3[sector].bch_result_x[j] > > > + val = > > > + readl(&gpmc_cfg->bch_result_0_3[0].bch_result_x[j] > > > ); > > > ecc_code[i++] = (val >> 24) & 0xFF; > > > ecc_code[i++] = (val >> 16) & 0xFF; @@ -431,22 > > > +430,6 @@ static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const > > u8 *dat, > > > return 0; > > > } > > > > > > -/** > > > - * omap_calculate_ecc_bch - ECC generator for 1 sector > > > - * @mtd: MTD device structure > > > - * @dat: The pointer to data on which ecc is computed > > > - * @ecc_code: The ecc_code buffer > > > - * > > > - * Support calculating of BCH4/8/16 ECC vectors for one sector. This > > > is used > > > - * when SW based correction is required as ECC is required for one > > > sector > > > - * at a time. > > > - */ > > > -static int __maybe_unused omap_calculate_ecc_bch(struct mtd_info *mtd, > > > - const u_char *dat, u_char *ecc_calc) > > > -{ > > > - return _omap_calculate_ecc_bch(mtd, dat, ecc_calc, 0); > > > -} > > > - > > > static inline void omap_nand_read_buf(struct mtd_info *mtd, uint8_t > > > *buf, int len) { > > > struct nand_chip *chip = mtd_to_nand(mtd); @@ -572,34 +555,6 > > > @@ static void omap_nand_read_prefetch(struct mtd_info *mtd, uint8_t > > > *buf, int len) > > > > > > #ifdef CONFIG_NAND_OMAP_ELM > > > > > > -/** > > > - * omap_calculate_ecc_bch_multi - Generate ECC for multiple sectors > > > - * @mtd: MTD device structure > > > - * @dat: The pointer to data on which ecc is computed > > > - * @ecc_code: The ecc_code buffer > > > - * > > > - * Support calculating of BCH4/8/16 ecc vectors for the entire page in one > > go. > > > - */ > > > -static int omap_calculate_ecc_bch_multi(struct mtd_info *mtd, > > > - const u_char *dat, u_char *ecc_calc) > > > -{ > > > - struct nand_chip *chip = mtd_to_nand(mtd); > > > - int eccbytes = chip->ecc.bytes; > > > - unsigned long nsectors; > > > - int i, ret; > > > - > > > - nsectors = ((readl(&gpmc_cfg->ecc_config) >> 4) & 0x7) + 1; > > > - for (i = 0; i < nsectors; i++) { > > > - ret = _omap_calculate_ecc_bch(mtd, dat, ecc_calc, i); > > > - if (ret) > > > - return ret; > > > - > > > - ecc_calc += eccbytes; > > > - } > > > - > > > - return 0; > > > -} > > > - > > > /* > > > * omap_reverse_list - re-orders list elements in reverse order [internal] > > > * @list: pointer to start of list > > > @@ -752,7 +707,6 @@ static int omap_read_page_bch(struct mtd_info > > > *mtd, struct nand_chip *chip, { > > > int i, eccsize = chip->ecc.size; > > > int eccbytes = chip->ecc.bytes; > > > - int ecctotal = chip->ecc.total; > > > int eccsteps = chip->ecc.steps; > > > uint8_t *p = buf; > > > uint8_t *ecc_calc = chip->buffers->ecccalc; @@ -760,24 +714,30 > > > @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip > > *chip, > > > uint32_t *eccpos = chip->ecc.layout->eccpos; > > > uint8_t *oob = chip->oob_poi; > > > uint32_t oob_pos; > > > + u32 data_pos = 0; > > > > > > /* oob area start */ > > > oob_pos = (eccsize * eccsteps) + chip->ecc.layout->eccpos[0]; > > > oob += chip->ecc.layout->eccpos[0]; > > > > > > - /* Enable ECC engine */ > > > - chip->ecc.hwctl(mtd, NAND_ECC_READ); > > > + for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize, > > > + oob += eccbytes) { > > > + /* Enable ECC engine */ > > > + chip->ecc.hwctl(mtd, NAND_ECC_READ); > > > > > > - /* read entire page */ > > > - chip->cmdfunc(mtd, NAND_CMD_RNDOUT, 0, -1); > > > - chip->read_buf(mtd, buf, mtd->writesize); > > > + /* read data */ > > > + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, data_pos, -1); > > > + chip->read_buf(mtd, p, eccsize); > > > > > > - /* read all ecc bytes from oob area */ > > > - chip->cmdfunc(mtd, NAND_CMD_RNDOUT, oob_pos, -1); > > > - chip->read_buf(mtd, oob, ecctotal); > > > + /* read respective ecc from oob area */ > > > + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, oob_pos, -1); > > > + chip->read_buf(mtd, oob, eccbytes); > > > + /* read syndrome */ > > > + chip->ecc.calculate(mtd, p, &ecc_calc[i]); > > > > > > - /* Calculate ecc bytes */ > > > - omap_calculate_ecc_bch_multi(mtd, buf, ecc_calc); > > > + data_pos += eccsize; > > > + oob_pos += eccbytes; > > > + } > > > > > > for (i = 0; i < chip->ecc.total; i++) > > > ecc_code[i] = chip->oob_poi[eccpos[i]]; @@ -945,6 > > > +905,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, > > > nand->ecc.hwctl = omap_enable_hwecc_bch; > > > nand->ecc.correct = omap_correct_data_bch_sw; > > > nand->ecc.calculate = omap_calculate_ecc_bch; > > > + nand->ecc.steps = eccsteps; > > > /* define ecc-layout */ > > > ecclayout->eccbytes = nand->ecc.bytes * eccsteps; > > > ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH; > > > @@ -987,6 +948,7 @@ static int omap_select_ecc_scheme(struct > > nand_chip *nand, > > > nand->ecc.correct = omap_correct_data_bch; > > > nand->ecc.calculate = omap_calculate_ecc_bch; > > > nand->ecc.read_page = omap_read_page_bch; > > > + nand->ecc.steps = eccsteps; > > > /* define ecc-layout */ > > > ecclayout->eccbytes = nand->ecc.bytes * eccsteps; > > > for (i = 0; i < ecclayout->eccbytes; i++) @@ -1020,6 > > > +982,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, > > > nand->ecc.correct = omap_correct_data_bch; > > > nand->ecc.calculate = omap_calculate_ecc_bch; > > > nand->ecc.read_page = omap_read_page_bch; > > > + nand->ecc.steps = eccsteps; > > > /* define ecc-layout */ > > > ecclayout->eccbytes = nand->ecc.bytes * eccsteps; > > > for (i = 0; i < ecclayout->eccbytes; i++) > > > > > > base-commit: 9e53e45292ee2f1d9d2ccc59914b161bef9b10d7 > > > -- > > > 2.34.1 > > > > > > > Let's wait on some tested-by > > I'm not a fan of this patch but we need to cover this regression Michael > > Michael
On 27/11/2023 16:43, Michael Nazzareno Trimarchi wrote: > Hi dario > > On Mon, Nov 27, 2023 at 3:00 PM Leto, Enrico <enrico.leto@siemens.com> wrote: >> >> Hi, >> >> Works on my draco thuban AM335x based boards booting from NAND with ECC BCH8 code. >> >> Tested-by: Enrico Leto <enrico.leto@siemens.com> >> >> Thanks >> >> >>> -----Original Message----- >>> From: Michael Nazzareno Trimarchi <michael@amarulasolutions.com> >>> Sent: Saturday, November 25, 2023 2:07 PM >>> To: Roger Quadros <rogerq@kernel.org> >>> Cc: dario.binacchi@amarulasolutions.com; Schocher, Heiko (EXT) (DENX >>> Software Engineering GmbH) <hs@denx.de>; Leto, Enrico (SI BP R&D ZG FW >>> CCP) <enrico.leto@siemens.com>; trini@konsulko.com; praneeth@ti.com; >>> nm@ti.com; vigneshr@ti.com; u-boot@lists.denx.de >>> Subject: Re: [PATCH] mtd: nand: omap_gpmc: Fix NAND in SPL for AM335x >>> >>> Hi Roger >>> >>> On Sat, Nov 25, 2023 at 12:16 PM Roger Quadros <rogerq@kernel.org> >>> wrote: >>>> >>>> AM335x uses a special driver "am335x_spl_bch.c" as SPL NAND loader. >>>> This driver expects 1 sector at a time ECC and doesn't work well with >>>> multi-sector ECC that was implemented in commit 04fcd2587321 ("mtd: >>>> rawnand: omap_gpmc: Fix BCH6/16 HW based correction") >>>> >>>> Switch back to 1 sector at a time read/ECC. >>>> >>>> Fixes: 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based >>>> correction") >>>> Signed-off-by: Roger Quadros <rogerq@kernel.org> >>>> --- >>>> drivers/mtd/nand/raw/omap_gpmc.c | 95 >>>> ++++++++++---------------------- >>>> 1 file changed, 29 insertions(+), 66 deletions(-) >>>> >>>> diff --git a/drivers/mtd/nand/raw/omap_gpmc.c >>>> b/drivers/mtd/nand/raw/omap_gpmc.c >>>> index 1a5ed0de31..2d2d2c2b6d 100644 >>>> --- a/drivers/mtd/nand/raw/omap_gpmc.c >>>> +++ b/drivers/mtd/nand/raw/omap_gpmc.c >>>> @@ -293,7 +293,7 @@ static void __maybe_unused >>> omap_enable_hwecc_bch(struct mtd_info *mtd, >>>> break; >>>> case OMAP_ECC_BCH8_CODE_HW: >>>> bch_type = 1; >>>> - nsectors = chip->ecc.steps; >>>> + nsectors = 1; >>>> if (mode == NAND_ECC_READ) { >>>> wr_mode = BCH_WRAPMODE_1; >>>> ecc_size0 = BCH8R_ECC_SIZE0; @@ -306,7 +306,7 >>>> @@ static void __maybe_unused omap_enable_hwecc_bch(struct mtd_info >>> *mtd, >>>> break; >>>> case OMAP_ECC_BCH16_CODE_HW: >>>> bch_type = 0x2; >>>> - nsectors = chip->ecc.steps; >>>> + nsectors = 1; >>>> if (mode == NAND_ECC_READ) { >>>> wr_mode = 0x01; >>>> ecc_size0 = 52; /* ECC bits in nibbles per >>>> sector */ @@ -345,17 +345,16 @@ static void __maybe_unused >>>> omap_enable_hwecc_bch(struct mtd_info *mtd, } >>>> >>>> /** >>>> - * _omap_calculate_ecc_bch - Generate BCH ECC bytes for one sector >>>> + * omap_calculate_ecc_bch - Generate BCH ECC bytes for one sector >>>> * @mtd: MTD device structure >>>> * @dat: The pointer to data on which ecc is computed >>>> * @ecc_code: The ecc_code buffer >>>> - * @sector: The sector number (for a multi sector page) >>>> * >>>> * Support calculating of BCH4/8/16 ECC vectors for one sector >>>> * within a page. Sector number is in @sector. >>>> */ >>>> -static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, >>>> - u8 *ecc_code, int sector) >>>> +static int omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, >>>> + u8 *ecc_code) >>>> { >>>> struct nand_chip *chip = mtd_to_nand(mtd); >>>> struct omap_nand_info *info = nand_get_controller_data(chip); >>>> @@ -368,7 +367,7 @@ static int _omap_calculate_ecc_bch(struct mtd_info >>> *mtd, const u8 *dat, >>>> case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW: >>>> #endif >>>> case OMAP_ECC_BCH8_CODE_HW: >>>> - ptr = &gpmc_cfg->bch_result_0_3[sector].bch_result_x[3]; >>>> + ptr = &gpmc_cfg->bch_result_0_3[0].bch_result_x[3]; >>>> val = readl(ptr); >>>> ecc_code[i++] = (val >> 0) & 0xFF; >>>> ptr--; >>>> @@ -383,21 +382,21 @@ static int _omap_calculate_ecc_bch(struct >>>> mtd_info *mtd, const u8 *dat, >>>> >>>> break; >>>> case OMAP_ECC_BCH16_CODE_HW: >>>> - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[2]); >>>> + val = >>>> + readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[2]); >>>> ecc_code[i++] = (val >> 8) & 0xFF; >>>> ecc_code[i++] = (val >> 0) & 0xFF; >>>> - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[1]); >>>> + val = >>>> + readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[1]); >>>> ecc_code[i++] = (val >> 24) & 0xFF; >>>> ecc_code[i++] = (val >> 16) & 0xFF; >>>> ecc_code[i++] = (val >> 8) & 0xFF; >>>> ecc_code[i++] = (val >> 0) & 0xFF; >>>> - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[0]); >>>> + val = >>>> + readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[0]); >>>> ecc_code[i++] = (val >> 24) & 0xFF; >>>> ecc_code[i++] = (val >> 16) & 0xFF; >>>> ecc_code[i++] = (val >> 8) & 0xFF; >>>> ecc_code[i++] = (val >> 0) & 0xFF; >>>> for (j = 3; j >= 0; j--) { >>>> - val = readl(&gpmc_cfg->bch_result_0_3[sector].bch_result_x[j] >>>> + val = >>>> + readl(&gpmc_cfg->bch_result_0_3[0].bch_result_x[j] >>>> ); >>>> ecc_code[i++] = (val >> 24) & 0xFF; >>>> ecc_code[i++] = (val >> 16) & 0xFF; @@ -431,22 >>>> +430,6 @@ static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const >>> u8 *dat, >>>> return 0; >>>> } >>>> >>>> -/** >>>> - * omap_calculate_ecc_bch - ECC generator for 1 sector >>>> - * @mtd: MTD device structure >>>> - * @dat: The pointer to data on which ecc is computed >>>> - * @ecc_code: The ecc_code buffer >>>> - * >>>> - * Support calculating of BCH4/8/16 ECC vectors for one sector. This >>>> is used >>>> - * when SW based correction is required as ECC is required for one >>>> sector >>>> - * at a time. >>>> - */ >>>> -static int __maybe_unused omap_calculate_ecc_bch(struct mtd_info *mtd, >>>> - const u_char *dat, u_char *ecc_calc) >>>> -{ >>>> - return _omap_calculate_ecc_bch(mtd, dat, ecc_calc, 0); >>>> -} >>>> - >>>> static inline void omap_nand_read_buf(struct mtd_info *mtd, uint8_t >>>> *buf, int len) { >>>> struct nand_chip *chip = mtd_to_nand(mtd); @@ -572,34 +555,6 >>>> @@ static void omap_nand_read_prefetch(struct mtd_info *mtd, uint8_t >>>> *buf, int len) >>>> >>>> #ifdef CONFIG_NAND_OMAP_ELM >>>> >>>> -/** >>>> - * omap_calculate_ecc_bch_multi - Generate ECC for multiple sectors >>>> - * @mtd: MTD device structure >>>> - * @dat: The pointer to data on which ecc is computed >>>> - * @ecc_code: The ecc_code buffer >>>> - * >>>> - * Support calculating of BCH4/8/16 ecc vectors for the entire page in one >>> go. >>>> - */ >>>> -static int omap_calculate_ecc_bch_multi(struct mtd_info *mtd, >>>> - const u_char *dat, u_char *ecc_calc) >>>> -{ >>>> - struct nand_chip *chip = mtd_to_nand(mtd); >>>> - int eccbytes = chip->ecc.bytes; >>>> - unsigned long nsectors; >>>> - int i, ret; >>>> - >>>> - nsectors = ((readl(&gpmc_cfg->ecc_config) >> 4) & 0x7) + 1; >>>> - for (i = 0; i < nsectors; i++) { >>>> - ret = _omap_calculate_ecc_bch(mtd, dat, ecc_calc, i); >>>> - if (ret) >>>> - return ret; >>>> - >>>> - ecc_calc += eccbytes; >>>> - } >>>> - >>>> - return 0; >>>> -} >>>> - >>>> /* >>>> * omap_reverse_list - re-orders list elements in reverse order [internal] >>>> * @list: pointer to start of list >>>> @@ -752,7 +707,6 @@ static int omap_read_page_bch(struct mtd_info >>>> *mtd, struct nand_chip *chip, { >>>> int i, eccsize = chip->ecc.size; >>>> int eccbytes = chip->ecc.bytes; >>>> - int ecctotal = chip->ecc.total; >>>> int eccsteps = chip->ecc.steps; >>>> uint8_t *p = buf; >>>> uint8_t *ecc_calc = chip->buffers->ecccalc; @@ -760,24 +714,30 >>>> @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip >>> *chip, >>>> uint32_t *eccpos = chip->ecc.layout->eccpos; >>>> uint8_t *oob = chip->oob_poi; >>>> uint32_t oob_pos; >>>> + u32 data_pos = 0; >>>> >>>> /* oob area start */ >>>> oob_pos = (eccsize * eccsteps) + chip->ecc.layout->eccpos[0]; >>>> oob += chip->ecc.layout->eccpos[0]; >>>> >>>> - /* Enable ECC engine */ >>>> - chip->ecc.hwctl(mtd, NAND_ECC_READ); >>>> + for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize, >>>> + oob += eccbytes) { >>>> + /* Enable ECC engine */ >>>> + chip->ecc.hwctl(mtd, NAND_ECC_READ); >>>> >>>> - /* read entire page */ >>>> - chip->cmdfunc(mtd, NAND_CMD_RNDOUT, 0, -1); >>>> - chip->read_buf(mtd, buf, mtd->writesize); >>>> + /* read data */ >>>> + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, data_pos, -1); >>>> + chip->read_buf(mtd, p, eccsize); >>>> >>>> - /* read all ecc bytes from oob area */ >>>> - chip->cmdfunc(mtd, NAND_CMD_RNDOUT, oob_pos, -1); >>>> - chip->read_buf(mtd, oob, ecctotal); >>>> + /* read respective ecc from oob area */ >>>> + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, oob_pos, -1); >>>> + chip->read_buf(mtd, oob, eccbytes); >>>> + /* read syndrome */ >>>> + chip->ecc.calculate(mtd, p, &ecc_calc[i]); >>>> >>>> - /* Calculate ecc bytes */ >>>> - omap_calculate_ecc_bch_multi(mtd, buf, ecc_calc); >>>> + data_pos += eccsize; >>>> + oob_pos += eccbytes; >>>> + } >>>> >>>> for (i = 0; i < chip->ecc.total; i++) >>>> ecc_code[i] = chip->oob_poi[eccpos[i]]; @@ -945,6 >>>> +905,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, >>>> nand->ecc.hwctl = omap_enable_hwecc_bch; >>>> nand->ecc.correct = omap_correct_data_bch_sw; >>>> nand->ecc.calculate = omap_calculate_ecc_bch; >>>> + nand->ecc.steps = eccsteps; >>>> /* define ecc-layout */ >>>> ecclayout->eccbytes = nand->ecc.bytes * eccsteps; >>>> ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH; >>>> @@ -987,6 +948,7 @@ static int omap_select_ecc_scheme(struct >>> nand_chip *nand, >>>> nand->ecc.correct = omap_correct_data_bch; >>>> nand->ecc.calculate = omap_calculate_ecc_bch; >>>> nand->ecc.read_page = omap_read_page_bch; >>>> + nand->ecc.steps = eccsteps; >>>> /* define ecc-layout */ >>>> ecclayout->eccbytes = nand->ecc.bytes * eccsteps; >>>> for (i = 0; i < ecclayout->eccbytes; i++) @@ -1020,6 >>>> +982,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, >>>> nand->ecc.correct = omap_correct_data_bch; >>>> nand->ecc.calculate = omap_calculate_ecc_bch; >>>> nand->ecc.read_page = omap_read_page_bch; >>>> + nand->ecc.steps = eccsteps; >>>> /* define ecc-layout */ >>>> ecclayout->eccbytes = nand->ecc.bytes * eccsteps; >>>> for (i = 0; i < ecclayout->eccbytes; i++) >>>> >>>> base-commit: 9e53e45292ee2f1d9d2ccc59914b161bef9b10d7 >>>> -- >>>> 2.34.1 >>>> >>> >>> Let's wait on some tested-by >>> > > I'm not a fan of this patch but we need to cover this regression Is it because of the performance hit for non AM335x platforms? Any suggestions for improvement?
Hi Tom, On 26/11/2023 19:35, Tom Rini wrote: > On Sat, Nov 25, 2023 at 01:16:05PM +0200, Roger Quadros wrote: > >> AM335x uses a special driver "am335x_spl_bch.c" as SPL >> NAND loader. This driver expects 1 sector at a time ECC >> and doesn't work well with multi-sector ECC that was implemented in >> commit 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based correction") >> >> Switch back to 1 sector at a time read/ECC. >> >> Fixes: 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based correction") >> Signed-off-by: Roger Quadros <rogerq@kernel.org> >> --- >> drivers/mtd/nand/raw/omap_gpmc.c | 95 ++++++++++---------------------- >> 1 file changed, 29 insertions(+), 66 deletions(-) > > I'm glad to see this fixed. My question is, can we abstract this > slightly as I assume there's a performance hit on the newer SoCs that > support more than one sector at a time for ECC and I assume it's just > am335x and related that don't support the feature. Thanks. > It looks like that the ELM driver (omap_elm.c) is not yet ready for multi-sector setup yet. I will need more time to test the multi-sector implementation.
On 25/11/2023 13:16, Roger Quadros wrote: > AM335x uses a special driver "am335x_spl_bch.c" as SPL > NAND loader. This driver expects 1 sector at a time ECC > and doesn't work well with multi-sector ECC that was implemented in > commit 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based correction") > > Switch back to 1 sector at a time read/ECC. > > Fixes: 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based correction") > Signed-off-by: Roger Quadros <rogerq@kernel.org> Azure pipeline build fails. Not because of this patch though. https://dev.azure.com/u-boot/u-boot/_build/results?buildId=7479&view=logs&j=c6c7c3ee-a125-5e20-d856-38cb989f4743&t=d274418e-7320-5c59-39b7-156cfcddae0b > --- > drivers/mtd/nand/raw/omap_gpmc.c | 95 ++++++++++---------------------- > 1 file changed, 29 insertions(+), 66 deletions(-) > > diff --git a/drivers/mtd/nand/raw/omap_gpmc.c b/drivers/mtd/nand/raw/omap_gpmc.c > index 1a5ed0de31..2d2d2c2b6d 100644 > --- a/drivers/mtd/nand/raw/omap_gpmc.c > +++ b/drivers/mtd/nand/raw/omap_gpmc.c > @@ -293,7 +293,7 @@ static void __maybe_unused omap_enable_hwecc_bch(struct mtd_info *mtd, > break; > case OMAP_ECC_BCH8_CODE_HW: > bch_type = 1; > - nsectors = chip->ecc.steps; > + nsectors = 1; > if (mode == NAND_ECC_READ) { > wr_mode = BCH_WRAPMODE_1; > ecc_size0 = BCH8R_ECC_SIZE0; > @@ -306,7 +306,7 @@ static void __maybe_unused omap_enable_hwecc_bch(struct mtd_info *mtd, > break; > case OMAP_ECC_BCH16_CODE_HW: > bch_type = 0x2; > - nsectors = chip->ecc.steps; > + nsectors = 1; > if (mode == NAND_ECC_READ) { > wr_mode = 0x01; > ecc_size0 = 52; /* ECC bits in nibbles per sector */ > @@ -345,17 +345,16 @@ static void __maybe_unused omap_enable_hwecc_bch(struct mtd_info *mtd, > } > > /** > - * _omap_calculate_ecc_bch - Generate BCH ECC bytes for one sector > + * omap_calculate_ecc_bch - Generate BCH ECC bytes for one sector > * @mtd: MTD device structure > * @dat: The pointer to data on which ecc is computed > * @ecc_code: The ecc_code buffer > - * @sector: The sector number (for a multi sector page) > * > * Support calculating of BCH4/8/16 ECC vectors for one sector > * within a page. Sector number is in @sector. > */ > -static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, > - u8 *ecc_code, int sector) > +static int omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, > + u8 *ecc_code) > { > struct nand_chip *chip = mtd_to_nand(mtd); > struct omap_nand_info *info = nand_get_controller_data(chip); > @@ -368,7 +367,7 @@ static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, > case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW: > #endif > case OMAP_ECC_BCH8_CODE_HW: > - ptr = &gpmc_cfg->bch_result_0_3[sector].bch_result_x[3]; > + ptr = &gpmc_cfg->bch_result_0_3[0].bch_result_x[3]; > val = readl(ptr); > ecc_code[i++] = (val >> 0) & 0xFF; > ptr--; > @@ -383,21 +382,21 @@ static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, > > break; > case OMAP_ECC_BCH16_CODE_HW: > - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[2]); > + val = readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[2]); > ecc_code[i++] = (val >> 8) & 0xFF; > ecc_code[i++] = (val >> 0) & 0xFF; > - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[1]); > + val = readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[1]); > ecc_code[i++] = (val >> 24) & 0xFF; > ecc_code[i++] = (val >> 16) & 0xFF; > ecc_code[i++] = (val >> 8) & 0xFF; > ecc_code[i++] = (val >> 0) & 0xFF; > - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[0]); > + val = readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[0]); > ecc_code[i++] = (val >> 24) & 0xFF; > ecc_code[i++] = (val >> 16) & 0xFF; > ecc_code[i++] = (val >> 8) & 0xFF; > ecc_code[i++] = (val >> 0) & 0xFF; > for (j = 3; j >= 0; j--) { > - val = readl(&gpmc_cfg->bch_result_0_3[sector].bch_result_x[j] > + val = readl(&gpmc_cfg->bch_result_0_3[0].bch_result_x[j] > ); > ecc_code[i++] = (val >> 24) & 0xFF; > ecc_code[i++] = (val >> 16) & 0xFF; > @@ -431,22 +430,6 @@ static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, > return 0; > } > > -/** > - * omap_calculate_ecc_bch - ECC generator for 1 sector > - * @mtd: MTD device structure > - * @dat: The pointer to data on which ecc is computed > - * @ecc_code: The ecc_code buffer > - * > - * Support calculating of BCH4/8/16 ECC vectors for one sector. This is used > - * when SW based correction is required as ECC is required for one sector > - * at a time. > - */ > -static int __maybe_unused omap_calculate_ecc_bch(struct mtd_info *mtd, > - const u_char *dat, u_char *ecc_calc) > -{ > - return _omap_calculate_ecc_bch(mtd, dat, ecc_calc, 0); > -} > - > static inline void omap_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len) > { > struct nand_chip *chip = mtd_to_nand(mtd); > @@ -572,34 +555,6 @@ static void omap_nand_read_prefetch(struct mtd_info *mtd, uint8_t *buf, int len) > > #ifdef CONFIG_NAND_OMAP_ELM > > -/** > - * omap_calculate_ecc_bch_multi - Generate ECC for multiple sectors > - * @mtd: MTD device structure > - * @dat: The pointer to data on which ecc is computed > - * @ecc_code: The ecc_code buffer > - * > - * Support calculating of BCH4/8/16 ecc vectors for the entire page in one go. > - */ > -static int omap_calculate_ecc_bch_multi(struct mtd_info *mtd, > - const u_char *dat, u_char *ecc_calc) > -{ > - struct nand_chip *chip = mtd_to_nand(mtd); > - int eccbytes = chip->ecc.bytes; > - unsigned long nsectors; > - int i, ret; > - > - nsectors = ((readl(&gpmc_cfg->ecc_config) >> 4) & 0x7) + 1; > - for (i = 0; i < nsectors; i++) { > - ret = _omap_calculate_ecc_bch(mtd, dat, ecc_calc, i); > - if (ret) > - return ret; > - > - ecc_calc += eccbytes; > - } > - > - return 0; > -} > - > /* > * omap_reverse_list - re-orders list elements in reverse order [internal] > * @list: pointer to start of list > @@ -752,7 +707,6 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip, > { > int i, eccsize = chip->ecc.size; > int eccbytes = chip->ecc.bytes; > - int ecctotal = chip->ecc.total; > int eccsteps = chip->ecc.steps; > uint8_t *p = buf; > uint8_t *ecc_calc = chip->buffers->ecccalc; > @@ -760,24 +714,30 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip, > uint32_t *eccpos = chip->ecc.layout->eccpos; > uint8_t *oob = chip->oob_poi; > uint32_t oob_pos; > + u32 data_pos = 0; > > /* oob area start */ > oob_pos = (eccsize * eccsteps) + chip->ecc.layout->eccpos[0]; > oob += chip->ecc.layout->eccpos[0]; > > - /* Enable ECC engine */ > - chip->ecc.hwctl(mtd, NAND_ECC_READ); > + for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize, > + oob += eccbytes) { > + /* Enable ECC engine */ > + chip->ecc.hwctl(mtd, NAND_ECC_READ); > > - /* read entire page */ > - chip->cmdfunc(mtd, NAND_CMD_RNDOUT, 0, -1); > - chip->read_buf(mtd, buf, mtd->writesize); > + /* read data */ > + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, data_pos, -1); > + chip->read_buf(mtd, p, eccsize); > > - /* read all ecc bytes from oob area */ > - chip->cmdfunc(mtd, NAND_CMD_RNDOUT, oob_pos, -1); > - chip->read_buf(mtd, oob, ecctotal); > + /* read respective ecc from oob area */ > + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, oob_pos, -1); > + chip->read_buf(mtd, oob, eccbytes); > + /* read syndrome */ > + chip->ecc.calculate(mtd, p, &ecc_calc[i]); > > - /* Calculate ecc bytes */ > - omap_calculate_ecc_bch_multi(mtd, buf, ecc_calc); > + data_pos += eccsize; > + oob_pos += eccbytes; > + } > > for (i = 0; i < chip->ecc.total; i++) > ecc_code[i] = chip->oob_poi[eccpos[i]]; > @@ -945,6 +905,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, > nand->ecc.hwctl = omap_enable_hwecc_bch; > nand->ecc.correct = omap_correct_data_bch_sw; > nand->ecc.calculate = omap_calculate_ecc_bch; > + nand->ecc.steps = eccsteps; > /* define ecc-layout */ > ecclayout->eccbytes = nand->ecc.bytes * eccsteps; > ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH; > @@ -987,6 +948,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, > nand->ecc.correct = omap_correct_data_bch; > nand->ecc.calculate = omap_calculate_ecc_bch; > nand->ecc.read_page = omap_read_page_bch; > + nand->ecc.steps = eccsteps; > /* define ecc-layout */ > ecclayout->eccbytes = nand->ecc.bytes * eccsteps; > for (i = 0; i < ecclayout->eccbytes; i++) > @@ -1020,6 +982,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, > nand->ecc.correct = omap_correct_data_bch; > nand->ecc.calculate = omap_calculate_ecc_bch; > nand->ecc.read_page = omap_read_page_bch; > + nand->ecc.steps = eccsteps; > /* define ecc-layout */ > ecclayout->eccbytes = nand->ecc.bytes * eccsteps; > for (i = 0; i < ecclayout->eccbytes; i++) > > base-commit: 9e53e45292ee2f1d9d2ccc59914b161bef9b10d7
Hi On Fri, Dec 8, 2023 at 9:32 AM Roger Quadros <rogerq@kernel.org> wrote: > > On 25/11/2023 13:16, Roger Quadros wrote: > > AM335x uses a special driver "am335x_spl_bch.c" as SPL > > NAND loader. This driver expects 1 sector at a time ECC > > and doesn't work well with multi-sector ECC that was implemented in > > commit 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based correction") > > > > Switch back to 1 sector at a time read/ECC. > > > > Fixes: 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based correction") > > Signed-off-by: Roger Quadros <rogerq@kernel.org> > > Azure pipeline build fails. Not because of this patch though. > https://dev.azure.com/u-boot/u-boot/_build/results?buildId=7479&view=logs&j=c6c7c3ee-a125-5e20-d856-38cb989f4743&t=d274418e-7320-5c59-39b7-156cfcddae0b > My comment below > > --- > > drivers/mtd/nand/raw/omap_gpmc.c | 95 ++++++++++---------------------- > > 1 file changed, 29 insertions(+), 66 deletions(-) > > > > diff --git a/drivers/mtd/nand/raw/omap_gpmc.c b/drivers/mtd/nand/raw/omap_gpmc.c > > index 1a5ed0de31..2d2d2c2b6d 100644 > > --- a/drivers/mtd/nand/raw/omap_gpmc.c > > +++ b/drivers/mtd/nand/raw/omap_gpmc.c > > @@ -293,7 +293,7 @@ static void __maybe_unused omap_enable_hwecc_bch(struct mtd_info *mtd, > > break; > > case OMAP_ECC_BCH8_CODE_HW: > > bch_type = 1; > > - nsectors = chip->ecc.steps; > > + nsectors = 1; > > if (mode == NAND_ECC_READ) { > > wr_mode = BCH_WRAPMODE_1; > > ecc_size0 = BCH8R_ECC_SIZE0; > > @@ -306,7 +306,7 @@ static void __maybe_unused omap_enable_hwecc_bch(struct mtd_info *mtd, > > break; > > case OMAP_ECC_BCH16_CODE_HW: > > bch_type = 0x2; > > - nsectors = chip->ecc.steps; > > + nsectors = 1; > > if (mode == NAND_ECC_READ) { > > wr_mode = 0x01; > > ecc_size0 = 52; /* ECC bits in nibbles per sector */ > > @@ -345,17 +345,16 @@ static void __maybe_unused omap_enable_hwecc_bch(struct mtd_info *mtd, > > } > > > > /** > > - * _omap_calculate_ecc_bch - Generate BCH ECC bytes for one sector > > + * omap_calculate_ecc_bch - Generate BCH ECC bytes for one sector > > * @mtd: MTD device structure > > * @dat: The pointer to data on which ecc is computed > > * @ecc_code: The ecc_code buffer > > - * @sector: The sector number (for a multi sector page) > > * > > * Support calculating of BCH4/8/16 ECC vectors for one sector > > * within a page. Sector number is in @sector. > > */ > > -static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, > > - u8 *ecc_code, int sector) > > +static int omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, > > + u8 *ecc_code) > > { This should be as before static int __maybe_unused omap_calculate_ecc_bch(struct mtd_info *mtd, > > struct nand_chip *chip = mtd_to_nand(mtd); > > struct omap_nand_info *info = nand_get_controller_data(chip); > > @@ -368,7 +367,7 @@ static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, > > case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW: > > #endif > > case OMAP_ECC_BCH8_CODE_HW: > > - ptr = &gpmc_cfg->bch_result_0_3[sector].bch_result_x[3]; > > + ptr = &gpmc_cfg->bch_result_0_3[0].bch_result_x[3]; > > val = readl(ptr); > > ecc_code[i++] = (val >> 0) & 0xFF; > > ptr--; > > @@ -383,21 +382,21 @@ static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, > > > > break; > > case OMAP_ECC_BCH16_CODE_HW: > > - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[2]); > > + val = readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[2]); > > ecc_code[i++] = (val >> 8) & 0xFF; > > ecc_code[i++] = (val >> 0) & 0xFF; > > - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[1]); > > + val = readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[1]); > > ecc_code[i++] = (val >> 24) & 0xFF; > > ecc_code[i++] = (val >> 16) & 0xFF; > > ecc_code[i++] = (val >> 8) & 0xFF; > > ecc_code[i++] = (val >> 0) & 0xFF; > > - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[0]); > > + val = readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[0]); > > ecc_code[i++] = (val >> 24) & 0xFF; > > ecc_code[i++] = (val >> 16) & 0xFF; > > ecc_code[i++] = (val >> 8) & 0xFF; > > ecc_code[i++] = (val >> 0) & 0xFF; > > for (j = 3; j >= 0; j--) { > > - val = readl(&gpmc_cfg->bch_result_0_3[sector].bch_result_x[j] > > + val = readl(&gpmc_cfg->bch_result_0_3[0].bch_result_x[j] > > ); > > ecc_code[i++] = (val >> 24) & 0xFF; > > ecc_code[i++] = (val >> 16) & 0xFF; > > @@ -431,22 +430,6 @@ static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, > > return 0; > > } > > > > -/** > > - * omap_calculate_ecc_bch - ECC generator for 1 sector > > - * @mtd: MTD device structure > > - * @dat: The pointer to data on which ecc is computed > > - * @ecc_code: The ecc_code buffer > > - * > > - * Support calculating of BCH4/8/16 ECC vectors for one sector. This is used > > - * when SW based correction is required as ECC is required for one sector > > - * at a time. > > - */ > > -static int __maybe_unused omap_calculate_ecc_bch(struct mtd_info *mtd, > > - const u_char *dat, u_char *ecc_calc) > > -{ If you remove you should stay with it > > - return _omap_calculate_ecc_bch(mtd, dat, ecc_calc, 0); > > -} > > - > > static inline void omap_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len) > > { > > struct nand_chip *chip = mtd_to_nand(mtd); > > @@ -572,34 +555,6 @@ static void omap_nand_read_prefetch(struct mtd_info *mtd, uint8_t *buf, int len) > > > > #ifdef CONFIG_NAND_OMAP_ELM > > > > -/** > > - * omap_calculate_ecc_bch_multi - Generate ECC for multiple sectors > > - * @mtd: MTD device structure > > - * @dat: The pointer to data on which ecc is computed > > - * @ecc_code: The ecc_code buffer > > - * > > - * Support calculating of BCH4/8/16 ecc vectors for the entire page in one go. > > - */ > > -static int omap_calculate_ecc_bch_multi(struct mtd_info *mtd, > > - const u_char *dat, u_char *ecc_calc) > > -{ > > - struct nand_chip *chip = mtd_to_nand(mtd); > > - int eccbytes = chip->ecc.bytes; > > - unsigned long nsectors; > > - int i, ret; > > - > > - nsectors = ((readl(&gpmc_cfg->ecc_config) >> 4) & 0x7) + 1; > > - for (i = 0; i < nsectors; i++) { > > - ret = _omap_calculate_ecc_bch(mtd, dat, ecc_calc, i); > > - if (ret) > > - return ret; > > - > > - ecc_calc += eccbytes; > > - } > > - > > - return 0; > > -} > > - > > /* > > * omap_reverse_list - re-orders list elements in reverse order [internal] > > * @list: pointer to start of list > > @@ -752,7 +707,6 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip, > > { > > int i, eccsize = chip->ecc.size; > > int eccbytes = chip->ecc.bytes; > > - int ecctotal = chip->ecc.total; > > int eccsteps = chip->ecc.steps; > > uint8_t *p = buf; > > uint8_t *ecc_calc = chip->buffers->ecccalc; > > @@ -760,24 +714,30 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip, > > uint32_t *eccpos = chip->ecc.layout->eccpos; > > uint8_t *oob = chip->oob_poi; > > uint32_t oob_pos; > > + u32 data_pos = 0; > > > > /* oob area start */ > > oob_pos = (eccsize * eccsteps) + chip->ecc.layout->eccpos[0]; > > oob += chip->ecc.layout->eccpos[0]; > > > > - /* Enable ECC engine */ > > - chip->ecc.hwctl(mtd, NAND_ECC_READ); > > + for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize, > > + oob += eccbytes) { > > + /* Enable ECC engine */ > > + chip->ecc.hwctl(mtd, NAND_ECC_READ); > > > > - /* read entire page */ > > - chip->cmdfunc(mtd, NAND_CMD_RNDOUT, 0, -1); > > - chip->read_buf(mtd, buf, mtd->writesize); > > + /* read data */ > > + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, data_pos, -1); > > + chip->read_buf(mtd, p, eccsize); > > > > - /* read all ecc bytes from oob area */ > > - chip->cmdfunc(mtd, NAND_CMD_RNDOUT, oob_pos, -1); > > - chip->read_buf(mtd, oob, ecctotal); > > + /* read respective ecc from oob area */ > > + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, oob_pos, -1); > > + chip->read_buf(mtd, oob, eccbytes); > > + /* read syndrome */ > > + chip->ecc.calculate(mtd, p, &ecc_calc[i]); > > > > - /* Calculate ecc bytes */ > > - omap_calculate_ecc_bch_multi(mtd, buf, ecc_calc); > > + data_pos += eccsize; > > + oob_pos += eccbytes; > > + } > > > > for (i = 0; i < chip->ecc.total; i++) > > ecc_code[i] = chip->oob_poi[eccpos[i]]; > > @@ -945,6 +905,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, > > nand->ecc.hwctl = omap_enable_hwecc_bch; > > nand->ecc.correct = omap_correct_data_bch_sw; > > nand->ecc.calculate = omap_calculate_ecc_bch; > > + nand->ecc.steps = eccsteps; > > /* define ecc-layout */ > > ecclayout->eccbytes = nand->ecc.bytes * eccsteps; > > ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH; > > @@ -987,6 +948,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, > > nand->ecc.correct = omap_correct_data_bch; > > nand->ecc.calculate = omap_calculate_ecc_bch; > > nand->ecc.read_page = omap_read_page_bch; > > + nand->ecc.steps = eccsteps; > > /* define ecc-layout */ > > ecclayout->eccbytes = nand->ecc.bytes * eccsteps; > > for (i = 0; i < ecclayout->eccbytes; i++) > > @@ -1020,6 +982,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, > > nand->ecc.correct = omap_correct_data_bch; > > nand->ecc.calculate = omap_calculate_ecc_bch; > > nand->ecc.read_page = omap_read_page_bch; > > + nand->ecc.steps = eccsteps; > > /* define ecc-layout */ > > ecclayout->eccbytes = nand->ecc.bytes * eccsteps; > > for (i = 0; i < ecclayout->eccbytes; i++) > > > > base-commit: 9e53e45292ee2f1d9d2ccc59914b161bef9b10d7 > > -- > cheers, > -roger
On 08/12/2023 10:41, Michael Nazzareno Trimarchi wrote: > Hi > > On Fri, Dec 8, 2023 at 9:32 AM Roger Quadros <rogerq@kernel.org> wrote: >> >> On 25/11/2023 13:16, Roger Quadros wrote: >>> AM335x uses a special driver "am335x_spl_bch.c" as SPL >>> NAND loader. This driver expects 1 sector at a time ECC >>> and doesn't work well with multi-sector ECC that was implemented in >>> commit 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based correction") >>> >>> Switch back to 1 sector at a time read/ECC. >>> >>> Fixes: 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based correction") >>> Signed-off-by: Roger Quadros <rogerq@kernel.org> >> >> Azure pipeline build fails. Not because of this patch though. >> https://dev.azure.com/u-boot/u-boot/_build/results?buildId=7479&view=logs&j=c6c7c3ee-a125-5e20-d856-38cb989f4743&t=d274418e-7320-5c59-39b7-156cfcddae0b >> > > My comment below > >>> --- >>> drivers/mtd/nand/raw/omap_gpmc.c | 95 ++++++++++---------------------- >>> 1 file changed, 29 insertions(+), 66 deletions(-) >>> >>> diff --git a/drivers/mtd/nand/raw/omap_gpmc.c b/drivers/mtd/nand/raw/omap_gpmc.c >>> index 1a5ed0de31..2d2d2c2b6d 100644 >>> --- a/drivers/mtd/nand/raw/omap_gpmc.c >>> +++ b/drivers/mtd/nand/raw/omap_gpmc.c >>> @@ -293,7 +293,7 @@ static void __maybe_unused omap_enable_hwecc_bch(struct mtd_info *mtd, >>> break; >>> case OMAP_ECC_BCH8_CODE_HW: >>> bch_type = 1; >>> - nsectors = chip->ecc.steps; >>> + nsectors = 1; >>> if (mode == NAND_ECC_READ) { >>> wr_mode = BCH_WRAPMODE_1; >>> ecc_size0 = BCH8R_ECC_SIZE0; >>> @@ -306,7 +306,7 @@ static void __maybe_unused omap_enable_hwecc_bch(struct mtd_info *mtd, >>> break; >>> case OMAP_ECC_BCH16_CODE_HW: >>> bch_type = 0x2; >>> - nsectors = chip->ecc.steps; >>> + nsectors = 1; >>> if (mode == NAND_ECC_READ) { >>> wr_mode = 0x01; >>> ecc_size0 = 52; /* ECC bits in nibbles per sector */ >>> @@ -345,17 +345,16 @@ static void __maybe_unused omap_enable_hwecc_bch(struct mtd_info *mtd, >>> } >>> >>> /** >>> - * _omap_calculate_ecc_bch - Generate BCH ECC bytes for one sector >>> + * omap_calculate_ecc_bch - Generate BCH ECC bytes for one sector >>> * @mtd: MTD device structure >>> * @dat: The pointer to data on which ecc is computed >>> * @ecc_code: The ecc_code buffer >>> - * @sector: The sector number (for a multi sector page) >>> * >>> * Support calculating of BCH4/8/16 ECC vectors for one sector >>> * within a page. Sector number is in @sector. >>> */ >>> -static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, >>> - u8 *ecc_code, int sector) >>> +static int omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, >>> + u8 *ecc_code) >>> { > > This should be as before > static int __maybe_unused omap_calculate_ecc_bch(struct mtd_info *mtd, > >>> struct nand_chip *chip = mtd_to_nand(mtd); >>> struct omap_nand_info *info = nand_get_controller_data(chip); >>> @@ -368,7 +367,7 @@ static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, >>> case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW: >>> #endif >>> case OMAP_ECC_BCH8_CODE_HW: >>> - ptr = &gpmc_cfg->bch_result_0_3[sector].bch_result_x[3]; >>> + ptr = &gpmc_cfg->bch_result_0_3[0].bch_result_x[3]; >>> val = readl(ptr); >>> ecc_code[i++] = (val >> 0) & 0xFF; >>> ptr--; >>> @@ -383,21 +382,21 @@ static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, >>> >>> break; >>> case OMAP_ECC_BCH16_CODE_HW: >>> - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[2]); >>> + val = readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[2]); >>> ecc_code[i++] = (val >> 8) & 0xFF; >>> ecc_code[i++] = (val >> 0) & 0xFF; >>> - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[1]); >>> + val = readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[1]); >>> ecc_code[i++] = (val >> 24) & 0xFF; >>> ecc_code[i++] = (val >> 16) & 0xFF; >>> ecc_code[i++] = (val >> 8) & 0xFF; >>> ecc_code[i++] = (val >> 0) & 0xFF; >>> - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[0]); >>> + val = readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[0]); >>> ecc_code[i++] = (val >> 24) & 0xFF; >>> ecc_code[i++] = (val >> 16) & 0xFF; >>> ecc_code[i++] = (val >> 8) & 0xFF; >>> ecc_code[i++] = (val >> 0) & 0xFF; >>> for (j = 3; j >= 0; j--) { >>> - val = readl(&gpmc_cfg->bch_result_0_3[sector].bch_result_x[j] >>> + val = readl(&gpmc_cfg->bch_result_0_3[0].bch_result_x[j] >>> ); >>> ecc_code[i++] = (val >> 24) & 0xFF; >>> ecc_code[i++] = (val >> 16) & 0xFF; >>> @@ -431,22 +430,6 @@ static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, >>> return 0; >>> } >>> >>> -/** >>> - * omap_calculate_ecc_bch - ECC generator for 1 sector >>> - * @mtd: MTD device structure >>> - * @dat: The pointer to data on which ecc is computed >>> - * @ecc_code: The ecc_code buffer >>> - * >>> - * Support calculating of BCH4/8/16 ECC vectors for one sector. This is used >>> - * when SW based correction is required as ECC is required for one sector >>> - * at a time. >>> - */ >>> -static int __maybe_unused omap_calculate_ecc_bch(struct mtd_info *mtd, >>> - const u_char *dat, u_char *ecc_calc) >>> -{ > > If you remove you should stay with it Thanks. So the issue was indeed introduced by this patch. I see a lot of #ifdeffery in this driver. Is it recommended in general to move to if defined(IS_ENABLED(CONFIG_*)) instead and get rid of __maybe_unused? > >>> - return _omap_calculate_ecc_bch(mtd, dat, ecc_calc, 0); >>> -} >>> - >>> static inline void omap_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len) >>> { >>> struct nand_chip *chip = mtd_to_nand(mtd); >>> @@ -572,34 +555,6 @@ static void omap_nand_read_prefetch(struct mtd_info *mtd, uint8_t *buf, int len) >>> >>> #ifdef CONFIG_NAND_OMAP_ELM >>> >>> -/** >>> - * omap_calculate_ecc_bch_multi - Generate ECC for multiple sectors >>> - * @mtd: MTD device structure >>> - * @dat: The pointer to data on which ecc is computed >>> - * @ecc_code: The ecc_code buffer >>> - * >>> - * Support calculating of BCH4/8/16 ecc vectors for the entire page in one go. >>> - */ >>> -static int omap_calculate_ecc_bch_multi(struct mtd_info *mtd, >>> - const u_char *dat, u_char *ecc_calc) >>> -{ >>> - struct nand_chip *chip = mtd_to_nand(mtd); >>> - int eccbytes = chip->ecc.bytes; >>> - unsigned long nsectors; >>> - int i, ret; >>> - >>> - nsectors = ((readl(&gpmc_cfg->ecc_config) >> 4) & 0x7) + 1; >>> - for (i = 0; i < nsectors; i++) { >>> - ret = _omap_calculate_ecc_bch(mtd, dat, ecc_calc, i); >>> - if (ret) >>> - return ret; >>> - >>> - ecc_calc += eccbytes; >>> - } >>> - >>> - return 0; >>> -} >>> - >>> /* >>> * omap_reverse_list - re-orders list elements in reverse order [internal] >>> * @list: pointer to start of list >>> @@ -752,7 +707,6 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip, >>> { >>> int i, eccsize = chip->ecc.size; >>> int eccbytes = chip->ecc.bytes; >>> - int ecctotal = chip->ecc.total; >>> int eccsteps = chip->ecc.steps; >>> uint8_t *p = buf; >>> uint8_t *ecc_calc = chip->buffers->ecccalc; >>> @@ -760,24 +714,30 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip, >>> uint32_t *eccpos = chip->ecc.layout->eccpos; >>> uint8_t *oob = chip->oob_poi; >>> uint32_t oob_pos; >>> + u32 data_pos = 0; >>> >>> /* oob area start */ >>> oob_pos = (eccsize * eccsteps) + chip->ecc.layout->eccpos[0]; >>> oob += chip->ecc.layout->eccpos[0]; >>> >>> - /* Enable ECC engine */ >>> - chip->ecc.hwctl(mtd, NAND_ECC_READ); >>> + for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize, >>> + oob += eccbytes) { >>> + /* Enable ECC engine */ >>> + chip->ecc.hwctl(mtd, NAND_ECC_READ); >>> >>> - /* read entire page */ >>> - chip->cmdfunc(mtd, NAND_CMD_RNDOUT, 0, -1); >>> - chip->read_buf(mtd, buf, mtd->writesize); >>> + /* read data */ >>> + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, data_pos, -1); >>> + chip->read_buf(mtd, p, eccsize); >>> >>> - /* read all ecc bytes from oob area */ >>> - chip->cmdfunc(mtd, NAND_CMD_RNDOUT, oob_pos, -1); >>> - chip->read_buf(mtd, oob, ecctotal); >>> + /* read respective ecc from oob area */ >>> + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, oob_pos, -1); >>> + chip->read_buf(mtd, oob, eccbytes); >>> + /* read syndrome */ >>> + chip->ecc.calculate(mtd, p, &ecc_calc[i]); >>> >>> - /* Calculate ecc bytes */ >>> - omap_calculate_ecc_bch_multi(mtd, buf, ecc_calc); >>> + data_pos += eccsize; >>> + oob_pos += eccbytes; >>> + } >>> >>> for (i = 0; i < chip->ecc.total; i++) >>> ecc_code[i] = chip->oob_poi[eccpos[i]]; >>> @@ -945,6 +905,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, >>> nand->ecc.hwctl = omap_enable_hwecc_bch; >>> nand->ecc.correct = omap_correct_data_bch_sw; >>> nand->ecc.calculate = omap_calculate_ecc_bch; >>> + nand->ecc.steps = eccsteps; >>> /* define ecc-layout */ >>> ecclayout->eccbytes = nand->ecc.bytes * eccsteps; >>> ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH; >>> @@ -987,6 +948,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, >>> nand->ecc.correct = omap_correct_data_bch; >>> nand->ecc.calculate = omap_calculate_ecc_bch; >>> nand->ecc.read_page = omap_read_page_bch; >>> + nand->ecc.steps = eccsteps; >>> /* define ecc-layout */ >>> ecclayout->eccbytes = nand->ecc.bytes * eccsteps; >>> for (i = 0; i < ecclayout->eccbytes; i++) >>> @@ -1020,6 +982,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, >>> nand->ecc.correct = omap_correct_data_bch; >>> nand->ecc.calculate = omap_calculate_ecc_bch; >>> nand->ecc.read_page = omap_read_page_bch; >>> + nand->ecc.steps = eccsteps; >>> /* define ecc-layout */ >>> ecclayout->eccbytes = nand->ecc.bytes * eccsteps; >>> for (i = 0; i < ecclayout->eccbytes; i++) >>> >>> base-commit: 9e53e45292ee2f1d9d2ccc59914b161bef9b10d7 >> >> -- >> cheers, >> -roger > > >
Hi Roger On Sat, Nov 25, 2023 at 12:16 PM Roger Quadros <rogerq@kernel.org> wrote: > > AM335x uses a special driver "am335x_spl_bch.c" as SPL > NAND loader. This driver expects 1 sector at a time ECC > and doesn't work well with multi-sector ECC that was implemented in > commit 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based correction") > > Switch back to 1 sector at a time read/ECC. > > Fixes: 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based correction") > Signed-off-by: Roger Quadros <rogerq@kernel.org> > --- > drivers/mtd/nand/raw/omap_gpmc.c | 95 ++++++++++---------------------- > 1 file changed, 29 insertions(+), 66 deletions(-) > > diff --git a/drivers/mtd/nand/raw/omap_gpmc.c b/drivers/mtd/nand/raw/omap_gpmc.c > index 1a5ed0de31..2d2d2c2b6d 100644 > --- a/drivers/mtd/nand/raw/omap_gpmc.c > +++ b/drivers/mtd/nand/raw/omap_gpmc.c > @@ -293,7 +293,7 @@ static void __maybe_unused omap_enable_hwecc_bch(struct mtd_info *mtd, > break; > case OMAP_ECC_BCH8_CODE_HW: > bch_type = 1; > - nsectors = chip->ecc.steps; > + nsectors = 1; > if (mode == NAND_ECC_READ) { > wr_mode = BCH_WRAPMODE_1; > ecc_size0 = BCH8R_ECC_SIZE0; > @@ -306,7 +306,7 @@ static void __maybe_unused omap_enable_hwecc_bch(struct mtd_info *mtd, > break; > case OMAP_ECC_BCH16_CODE_HW: > bch_type = 0x2; > - nsectors = chip->ecc.steps; > + nsectors = 1; > if (mode == NAND_ECC_READ) { > wr_mode = 0x01; > ecc_size0 = 52; /* ECC bits in nibbles per sector */ > @@ -345,17 +345,16 @@ static void __maybe_unused omap_enable_hwecc_bch(struct mtd_info *mtd, > } > If the changes impact only one family can you just restrict it to those family? Michael > /** > - * _omap_calculate_ecc_bch - Generate BCH ECC bytes for one sector > + * omap_calculate_ecc_bch - Generate BCH ECC bytes for one sector > * @mtd: MTD device structure > * @dat: The pointer to data on which ecc is computed > * @ecc_code: The ecc_code buffer > - * @sector: The sector number (for a multi sector page) > * > * Support calculating of BCH4/8/16 ECC vectors for one sector > * within a page. Sector number is in @sector. > */ > -static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, > - u8 *ecc_code, int sector) > +static int omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, > + u8 *ecc_code) > { > struct nand_chip *chip = mtd_to_nand(mtd); > struct omap_nand_info *info = nand_get_controller_data(chip); > @@ -368,7 +367,7 @@ static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, > case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW: > #endif > case OMAP_ECC_BCH8_CODE_HW: > - ptr = &gpmc_cfg->bch_result_0_3[sector].bch_result_x[3]; > + ptr = &gpmc_cfg->bch_result_0_3[0].bch_result_x[3]; > val = readl(ptr); > ecc_code[i++] = (val >> 0) & 0xFF; > ptr--; > @@ -383,21 +382,21 @@ static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, > > break; > case OMAP_ECC_BCH16_CODE_HW: > - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[2]); > + val = readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[2]); > ecc_code[i++] = (val >> 8) & 0xFF; > ecc_code[i++] = (val >> 0) & 0xFF; > - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[1]); > + val = readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[1]); > ecc_code[i++] = (val >> 24) & 0xFF; > ecc_code[i++] = (val >> 16) & 0xFF; > ecc_code[i++] = (val >> 8) & 0xFF; > ecc_code[i++] = (val >> 0) & 0xFF; > - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[0]); > + val = readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[0]); > ecc_code[i++] = (val >> 24) & 0xFF; > ecc_code[i++] = (val >> 16) & 0xFF; > ecc_code[i++] = (val >> 8) & 0xFF; > ecc_code[i++] = (val >> 0) & 0xFF; > for (j = 3; j >= 0; j--) { > - val = readl(&gpmc_cfg->bch_result_0_3[sector].bch_result_x[j] > + val = readl(&gpmc_cfg->bch_result_0_3[0].bch_result_x[j] > ); > ecc_code[i++] = (val >> 24) & 0xFF; > ecc_code[i++] = (val >> 16) & 0xFF; > @@ -431,22 +430,6 @@ static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, > return 0; > } > > -/** > - * omap_calculate_ecc_bch - ECC generator for 1 sector > - * @mtd: MTD device structure > - * @dat: The pointer to data on which ecc is computed > - * @ecc_code: The ecc_code buffer > - * > - * Support calculating of BCH4/8/16 ECC vectors for one sector. This is used > - * when SW based correction is required as ECC is required for one sector > - * at a time. > - */ > -static int __maybe_unused omap_calculate_ecc_bch(struct mtd_info *mtd, > - const u_char *dat, u_char *ecc_calc) > -{ > - return _omap_calculate_ecc_bch(mtd, dat, ecc_calc, 0); > -} > - > static inline void omap_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len) > { > struct nand_chip *chip = mtd_to_nand(mtd); > @@ -572,34 +555,6 @@ static void omap_nand_read_prefetch(struct mtd_info *mtd, uint8_t *buf, int len) > > #ifdef CONFIG_NAND_OMAP_ELM > > -/** > - * omap_calculate_ecc_bch_multi - Generate ECC for multiple sectors > - * @mtd: MTD device structure > - * @dat: The pointer to data on which ecc is computed > - * @ecc_code: The ecc_code buffer > - * > - * Support calculating of BCH4/8/16 ecc vectors for the entire page in one go. > - */ > -static int omap_calculate_ecc_bch_multi(struct mtd_info *mtd, > - const u_char *dat, u_char *ecc_calc) > -{ > - struct nand_chip *chip = mtd_to_nand(mtd); > - int eccbytes = chip->ecc.bytes; > - unsigned long nsectors; > - int i, ret; > - > - nsectors = ((readl(&gpmc_cfg->ecc_config) >> 4) & 0x7) + 1; > - for (i = 0; i < nsectors; i++) { > - ret = _omap_calculate_ecc_bch(mtd, dat, ecc_calc, i); > - if (ret) > - return ret; > - > - ecc_calc += eccbytes; > - } > - > - return 0; > -} > - > /* > * omap_reverse_list - re-orders list elements in reverse order [internal] > * @list: pointer to start of list > @@ -752,7 +707,6 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip, > { > int i, eccsize = chip->ecc.size; > int eccbytes = chip->ecc.bytes; > - int ecctotal = chip->ecc.total; > int eccsteps = chip->ecc.steps; > uint8_t *p = buf; > uint8_t *ecc_calc = chip->buffers->ecccalc; > @@ -760,24 +714,30 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip, > uint32_t *eccpos = chip->ecc.layout->eccpos; > uint8_t *oob = chip->oob_poi; > uint32_t oob_pos; > + u32 data_pos = 0; > > /* oob area start */ > oob_pos = (eccsize * eccsteps) + chip->ecc.layout->eccpos[0]; > oob += chip->ecc.layout->eccpos[0]; > > - /* Enable ECC engine */ > - chip->ecc.hwctl(mtd, NAND_ECC_READ); > + for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize, > + oob += eccbytes) { > + /* Enable ECC engine */ > + chip->ecc.hwctl(mtd, NAND_ECC_READ); > > - /* read entire page */ > - chip->cmdfunc(mtd, NAND_CMD_RNDOUT, 0, -1); > - chip->read_buf(mtd, buf, mtd->writesize); > + /* read data */ > + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, data_pos, -1); > + chip->read_buf(mtd, p, eccsize); > > - /* read all ecc bytes from oob area */ > - chip->cmdfunc(mtd, NAND_CMD_RNDOUT, oob_pos, -1); > - chip->read_buf(mtd, oob, ecctotal); > + /* read respective ecc from oob area */ > + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, oob_pos, -1); > + chip->read_buf(mtd, oob, eccbytes); > + /* read syndrome */ > + chip->ecc.calculate(mtd, p, &ecc_calc[i]); > > - /* Calculate ecc bytes */ > - omap_calculate_ecc_bch_multi(mtd, buf, ecc_calc); > + data_pos += eccsize; > + oob_pos += eccbytes; > + } > > for (i = 0; i < chip->ecc.total; i++) > ecc_code[i] = chip->oob_poi[eccpos[i]]; > @@ -945,6 +905,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, > nand->ecc.hwctl = omap_enable_hwecc_bch; > nand->ecc.correct = omap_correct_data_bch_sw; > nand->ecc.calculate = omap_calculate_ecc_bch; > + nand->ecc.steps = eccsteps; > /* define ecc-layout */ > ecclayout->eccbytes = nand->ecc.bytes * eccsteps; > ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH; > @@ -987,6 +948,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, > nand->ecc.correct = omap_correct_data_bch; > nand->ecc.calculate = omap_calculate_ecc_bch; > nand->ecc.read_page = omap_read_page_bch; > + nand->ecc.steps = eccsteps; > /* define ecc-layout */ > ecclayout->eccbytes = nand->ecc.bytes * eccsteps; > for (i = 0; i < ecclayout->eccbytes; i++) > @@ -1020,6 +982,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, > nand->ecc.correct = omap_correct_data_bch; > nand->ecc.calculate = omap_calculate_ecc_bch; > nand->ecc.read_page = omap_read_page_bch; > + nand->ecc.steps = eccsteps; > /* define ecc-layout */ > ecclayout->eccbytes = nand->ecc.bytes * eccsteps; > for (i = 0; i < ecclayout->eccbytes; i++) > > base-commit: 9e53e45292ee2f1d9d2ccc59914b161bef9b10d7 > -- > 2.34.1 >
On Fri, Dec 08, 2023 at 10:31:54AM +0200, Roger Quadros wrote: > On 25/11/2023 13:16, Roger Quadros wrote: > > AM335x uses a special driver "am335x_spl_bch.c" as SPL > > NAND loader. This driver expects 1 sector at a time ECC > > and doesn't work well with multi-sector ECC that was implemented in > > commit 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based correction") > > > > Switch back to 1 sector at a time read/ECC. > > > > Fixes: 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based correction") > > Signed-off-by: Roger Quadros <rogerq@kernel.org> > > Azure pipeline build fails. Not because of this patch though. > https://dev.azure.com/u-boot/u-boot/_build/results?buildId=7479&view=logs&j=c6c7c3ee-a125-5e20-d856-38cb989f4743&t=d274418e-7320-5c59-39b7-156cfcddae0b Er? The failure is just above the end of the page (which the link opens to for me) and is: +drivers/mtd/nand/raw/omap_gpmc.c:356:12: error: 'omap_calculate_ecc_bch' defined but not used [-Werror=unused-function] + 356 | static int omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, + | ^~~~~~~~~~~~~~~~~~~~~~ +cc1: all warnings being treated as errors And is this patch :)
On Thu, Dec 07, 2023 at 04:22:42PM +0200, Roger Quadros wrote: > Hi Tom, > > On 26/11/2023 19:35, Tom Rini wrote: > > On Sat, Nov 25, 2023 at 01:16:05PM +0200, Roger Quadros wrote: > > > >> AM335x uses a special driver "am335x_spl_bch.c" as SPL > >> NAND loader. This driver expects 1 sector at a time ECC > >> and doesn't work well with multi-sector ECC that was implemented in > >> commit 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based correction") > >> > >> Switch back to 1 sector at a time read/ECC. > >> > >> Fixes: 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based correction") > >> Signed-off-by: Roger Quadros <rogerq@kernel.org> > >> --- > >> drivers/mtd/nand/raw/omap_gpmc.c | 95 ++++++++++---------------------- > >> 1 file changed, 29 insertions(+), 66 deletions(-) > > > > I'm glad to see this fixed. My question is, can we abstract this > > slightly as I assume there's a performance hit on the newer SoCs that > > support more than one sector at a time for ECC and I assume it's just > > am335x and related that don't support the feature. Thanks. > > > > It looks like that the ELM driver (omap_elm.c) is not yet ready for multi-sector setup yet. > I will need more time to test the multi-sector implementation. Ah, thanks. Lets make that clearer in the commit message of the next spin please.
diff --git a/drivers/mtd/nand/raw/omap_gpmc.c b/drivers/mtd/nand/raw/omap_gpmc.c index 1a5ed0de31..2d2d2c2b6d 100644 --- a/drivers/mtd/nand/raw/omap_gpmc.c +++ b/drivers/mtd/nand/raw/omap_gpmc.c @@ -293,7 +293,7 @@ static void __maybe_unused omap_enable_hwecc_bch(struct mtd_info *mtd, break; case OMAP_ECC_BCH8_CODE_HW: bch_type = 1; - nsectors = chip->ecc.steps; + nsectors = 1; if (mode == NAND_ECC_READ) { wr_mode = BCH_WRAPMODE_1; ecc_size0 = BCH8R_ECC_SIZE0; @@ -306,7 +306,7 @@ static void __maybe_unused omap_enable_hwecc_bch(struct mtd_info *mtd, break; case OMAP_ECC_BCH16_CODE_HW: bch_type = 0x2; - nsectors = chip->ecc.steps; + nsectors = 1; if (mode == NAND_ECC_READ) { wr_mode = 0x01; ecc_size0 = 52; /* ECC bits in nibbles per sector */ @@ -345,17 +345,16 @@ static void __maybe_unused omap_enable_hwecc_bch(struct mtd_info *mtd, } /** - * _omap_calculate_ecc_bch - Generate BCH ECC bytes for one sector + * omap_calculate_ecc_bch - Generate BCH ECC bytes for one sector * @mtd: MTD device structure * @dat: The pointer to data on which ecc is computed * @ecc_code: The ecc_code buffer - * @sector: The sector number (for a multi sector page) * * Support calculating of BCH4/8/16 ECC vectors for one sector * within a page. Sector number is in @sector. */ -static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, - u8 *ecc_code, int sector) +static int omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, + u8 *ecc_code) { struct nand_chip *chip = mtd_to_nand(mtd); struct omap_nand_info *info = nand_get_controller_data(chip); @@ -368,7 +367,7 @@ static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW: #endif case OMAP_ECC_BCH8_CODE_HW: - ptr = &gpmc_cfg->bch_result_0_3[sector].bch_result_x[3]; + ptr = &gpmc_cfg->bch_result_0_3[0].bch_result_x[3]; val = readl(ptr); ecc_code[i++] = (val >> 0) & 0xFF; ptr--; @@ -383,21 +382,21 @@ static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, break; case OMAP_ECC_BCH16_CODE_HW: - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[2]); + val = readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[2]); ecc_code[i++] = (val >> 8) & 0xFF; ecc_code[i++] = (val >> 0) & 0xFF; - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[1]); + val = readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[1]); ecc_code[i++] = (val >> 24) & 0xFF; ecc_code[i++] = (val >> 16) & 0xFF; ecc_code[i++] = (val >> 8) & 0xFF; ecc_code[i++] = (val >> 0) & 0xFF; - val = readl(&gpmc_cfg->bch_result_4_6[sector].bch_result_x[0]); + val = readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[0]); ecc_code[i++] = (val >> 24) & 0xFF; ecc_code[i++] = (val >> 16) & 0xFF; ecc_code[i++] = (val >> 8) & 0xFF; ecc_code[i++] = (val >> 0) & 0xFF; for (j = 3; j >= 0; j--) { - val = readl(&gpmc_cfg->bch_result_0_3[sector].bch_result_x[j] + val = readl(&gpmc_cfg->bch_result_0_3[0].bch_result_x[j] ); ecc_code[i++] = (val >> 24) & 0xFF; ecc_code[i++] = (val >> 16) & 0xFF; @@ -431,22 +430,6 @@ static int _omap_calculate_ecc_bch(struct mtd_info *mtd, const u8 *dat, return 0; } -/** - * omap_calculate_ecc_bch - ECC generator for 1 sector - * @mtd: MTD device structure - * @dat: The pointer to data on which ecc is computed - * @ecc_code: The ecc_code buffer - * - * Support calculating of BCH4/8/16 ECC vectors for one sector. This is used - * when SW based correction is required as ECC is required for one sector - * at a time. - */ -static int __maybe_unused omap_calculate_ecc_bch(struct mtd_info *mtd, - const u_char *dat, u_char *ecc_calc) -{ - return _omap_calculate_ecc_bch(mtd, dat, ecc_calc, 0); -} - static inline void omap_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len) { struct nand_chip *chip = mtd_to_nand(mtd); @@ -572,34 +555,6 @@ static void omap_nand_read_prefetch(struct mtd_info *mtd, uint8_t *buf, int len) #ifdef CONFIG_NAND_OMAP_ELM -/** - * omap_calculate_ecc_bch_multi - Generate ECC for multiple sectors - * @mtd: MTD device structure - * @dat: The pointer to data on which ecc is computed - * @ecc_code: The ecc_code buffer - * - * Support calculating of BCH4/8/16 ecc vectors for the entire page in one go. - */ -static int omap_calculate_ecc_bch_multi(struct mtd_info *mtd, - const u_char *dat, u_char *ecc_calc) -{ - struct nand_chip *chip = mtd_to_nand(mtd); - int eccbytes = chip->ecc.bytes; - unsigned long nsectors; - int i, ret; - - nsectors = ((readl(&gpmc_cfg->ecc_config) >> 4) & 0x7) + 1; - for (i = 0; i < nsectors; i++) { - ret = _omap_calculate_ecc_bch(mtd, dat, ecc_calc, i); - if (ret) - return ret; - - ecc_calc += eccbytes; - } - - return 0; -} - /* * omap_reverse_list - re-orders list elements in reverse order [internal] * @list: pointer to start of list @@ -752,7 +707,6 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip, { int i, eccsize = chip->ecc.size; int eccbytes = chip->ecc.bytes; - int ecctotal = chip->ecc.total; int eccsteps = chip->ecc.steps; uint8_t *p = buf; uint8_t *ecc_calc = chip->buffers->ecccalc; @@ -760,24 +714,30 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip, uint32_t *eccpos = chip->ecc.layout->eccpos; uint8_t *oob = chip->oob_poi; uint32_t oob_pos; + u32 data_pos = 0; /* oob area start */ oob_pos = (eccsize * eccsteps) + chip->ecc.layout->eccpos[0]; oob += chip->ecc.layout->eccpos[0]; - /* Enable ECC engine */ - chip->ecc.hwctl(mtd, NAND_ECC_READ); + for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize, + oob += eccbytes) { + /* Enable ECC engine */ + chip->ecc.hwctl(mtd, NAND_ECC_READ); - /* read entire page */ - chip->cmdfunc(mtd, NAND_CMD_RNDOUT, 0, -1); - chip->read_buf(mtd, buf, mtd->writesize); + /* read data */ + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, data_pos, -1); + chip->read_buf(mtd, p, eccsize); - /* read all ecc bytes from oob area */ - chip->cmdfunc(mtd, NAND_CMD_RNDOUT, oob_pos, -1); - chip->read_buf(mtd, oob, ecctotal); + /* read respective ecc from oob area */ + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, oob_pos, -1); + chip->read_buf(mtd, oob, eccbytes); + /* read syndrome */ + chip->ecc.calculate(mtd, p, &ecc_calc[i]); - /* Calculate ecc bytes */ - omap_calculate_ecc_bch_multi(mtd, buf, ecc_calc); + data_pos += eccsize; + oob_pos += eccbytes; + } for (i = 0; i < chip->ecc.total; i++) ecc_code[i] = chip->oob_poi[eccpos[i]]; @@ -945,6 +905,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, nand->ecc.hwctl = omap_enable_hwecc_bch; nand->ecc.correct = omap_correct_data_bch_sw; nand->ecc.calculate = omap_calculate_ecc_bch; + nand->ecc.steps = eccsteps; /* define ecc-layout */ ecclayout->eccbytes = nand->ecc.bytes * eccsteps; ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH; @@ -987,6 +948,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, nand->ecc.correct = omap_correct_data_bch; nand->ecc.calculate = omap_calculate_ecc_bch; nand->ecc.read_page = omap_read_page_bch; + nand->ecc.steps = eccsteps; /* define ecc-layout */ ecclayout->eccbytes = nand->ecc.bytes * eccsteps; for (i = 0; i < ecclayout->eccbytes; i++) @@ -1020,6 +982,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, nand->ecc.correct = omap_correct_data_bch; nand->ecc.calculate = omap_calculate_ecc_bch; nand->ecc.read_page = omap_read_page_bch; + nand->ecc.steps = eccsteps; /* define ecc-layout */ ecclayout->eccbytes = nand->ecc.bytes * eccsteps; for (i = 0; i < ecclayout->eccbytes; i++)
AM335x uses a special driver "am335x_spl_bch.c" as SPL NAND loader. This driver expects 1 sector at a time ECC and doesn't work well with multi-sector ECC that was implemented in commit 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based correction") Switch back to 1 sector at a time read/ECC. Fixes: 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based correction") Signed-off-by: Roger Quadros <rogerq@kernel.org> --- drivers/mtd/nand/raw/omap_gpmc.c | 95 ++++++++++---------------------- 1 file changed, 29 insertions(+), 66 deletions(-) base-commit: 9e53e45292ee2f1d9d2ccc59914b161bef9b10d7