* [PATCH] mtd: nand: omap_gpmc: Fix NAND in SPL for AM335x
@ 2023-11-25 11:16 Roger Quadros
2023-11-25 13:06 ` Michael Nazzareno Trimarchi
` (4 more replies)
0 siblings, 5 replies; 14+ messages in thread
From: Roger Quadros @ 2023-11-25 11:16 UTC (permalink / raw)
To: dario.binacchi, michael, hs
Cc: enrico.leto, trini, praneeth, nm, vigneshr, u-boot, Roger Quadros
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: nand: omap_gpmc: Fix NAND in SPL for AM335x
2023-11-25 11:16 [PATCH] mtd: nand: omap_gpmc: Fix NAND in SPL for AM335x Roger Quadros
@ 2023-11-25 13:06 ` Michael Nazzareno Trimarchi
2023-11-27 14:00 ` Leto, Enrico
2023-11-26 17:35 ` Tom Rini
` (3 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Michael Nazzareno Trimarchi @ 2023-11-25 13:06 UTC (permalink / raw)
To: Roger Quadros
Cc: dario.binacchi, hs, enrico.leto, trini, praneeth, nm, vigneshr,
u-boot
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: nand: omap_gpmc: Fix NAND in SPL for AM335x
2023-11-25 11:16 [PATCH] mtd: nand: omap_gpmc: Fix NAND in SPL for AM335x Roger Quadros
2023-11-25 13:06 ` Michael Nazzareno Trimarchi
@ 2023-11-26 17:35 ` Tom Rini
2023-12-07 14:22 ` Roger Quadros
2023-11-27 13:39 ` Heiko Schocher
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Tom Rini @ 2023-11-26 17:35 UTC (permalink / raw)
To: Roger Quadros
Cc: dario.binacchi, michael, hs, enrico.leto, praneeth, nm, vigneshr,
u-boot
[-- Attachment #1: Type: text/plain, Size: 947 bytes --]
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.
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: nand: omap_gpmc: Fix NAND in SPL for AM335x
2023-11-25 11:16 [PATCH] mtd: nand: omap_gpmc: Fix NAND in SPL for AM335x Roger Quadros
2023-11-25 13:06 ` Michael Nazzareno Trimarchi
2023-11-26 17:35 ` Tom Rini
@ 2023-11-27 13:39 ` Heiko Schocher
2023-12-08 8:31 ` Roger Quadros
2023-12-08 16:22 ` Michael Nazzareno Trimarchi
4 siblings, 0 replies; 14+ messages in thread
From: Heiko Schocher @ 2023-11-27 13:39 UTC (permalink / raw)
To: Roger Quadros, dario.binacchi, michael
Cc: enrico.leto, trini, praneeth, nm, vigneshr, u-boot
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
--
DENX Software Engineering GmbH, Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@denx.de
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH] mtd: nand: omap_gpmc: Fix NAND in SPL for AM335x
2023-11-25 13:06 ` Michael Nazzareno Trimarchi
@ 2023-11-27 14:00 ` Leto, Enrico
2023-11-27 14:43 ` Michael Nazzareno Trimarchi
0 siblings, 1 reply; 14+ messages in thread
From: Leto, Enrico @ 2023-11-27 14:00 UTC (permalink / raw)
To: 'Michael Nazzareno Trimarchi', Roger Quadros
Cc: dario.binacchi@amarulasolutions.com, hs@denx.de,
trini@konsulko.com, praneeth@ti.com, nm@ti.com, vigneshr@ti.com,
u-boot@lists.denx.de
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: nand: omap_gpmc: Fix NAND in SPL for AM335x
2023-11-27 14:00 ` Leto, Enrico
@ 2023-11-27 14:43 ` Michael Nazzareno Trimarchi
2023-11-28 9:21 ` Roger Quadros
0 siblings, 1 reply; 14+ messages in thread
From: Michael Nazzareno Trimarchi @ 2023-11-27 14:43 UTC (permalink / raw)
To: Leto, Enrico
Cc: Roger Quadros, dario.binacchi@amarulasolutions.com, hs@denx.de,
trini@konsulko.com, praneeth@ti.com, nm@ti.com, vigneshr@ti.com,
u-boot@lists.denx.de
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
--
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________
Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: nand: omap_gpmc: Fix NAND in SPL for AM335x
2023-11-27 14:43 ` Michael Nazzareno Trimarchi
@ 2023-11-28 9:21 ` Roger Quadros
0 siblings, 0 replies; 14+ messages in thread
From: Roger Quadros @ 2023-11-28 9:21 UTC (permalink / raw)
To: Michael Nazzareno Trimarchi, Leto, Enrico
Cc: dario.binacchi@amarulasolutions.com, hs@denx.de,
trini@konsulko.com, praneeth@ti.com, nm@ti.com, vigneshr@ti.com,
u-boot@lists.denx.de
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?
--
cheers,
-roger
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: nand: omap_gpmc: Fix NAND in SPL for AM335x
2023-11-26 17:35 ` Tom Rini
@ 2023-12-07 14:22 ` Roger Quadros
2023-12-09 19:51 ` Tom Rini
0 siblings, 1 reply; 14+ messages in thread
From: Roger Quadros @ 2023-12-07 14:22 UTC (permalink / raw)
To: Tom Rini
Cc: dario.binacchi, michael, hs, enrico.leto, praneeth, nm, vigneshr,
u-boot
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.
--
cheers,
-roger
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: nand: omap_gpmc: Fix NAND in SPL for AM335x
2023-11-25 11:16 [PATCH] mtd: nand: omap_gpmc: Fix NAND in SPL for AM335x Roger Quadros
` (2 preceding siblings ...)
2023-11-27 13:39 ` Heiko Schocher
@ 2023-12-08 8:31 ` Roger Quadros
2023-12-08 8:41 ` Michael Nazzareno Trimarchi
2023-12-09 19:50 ` Tom Rini
2023-12-08 16:22 ` Michael Nazzareno Trimarchi
4 siblings, 2 replies; 14+ messages in thread
From: Roger Quadros @ 2023-12-08 8:31 UTC (permalink / raw)
To: dario.binacchi, michael, hs
Cc: enrico.leto, trini, praneeth, nm, vigneshr, u-boot
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
--
cheers,
-roger
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: nand: omap_gpmc: Fix NAND in SPL for AM335x
2023-12-08 8:31 ` Roger Quadros
@ 2023-12-08 8:41 ` Michael Nazzareno Trimarchi
2023-12-08 8:44 ` Roger Quadros
2023-12-09 19:50 ` Tom Rini
1 sibling, 1 reply; 14+ messages in thread
From: Michael Nazzareno Trimarchi @ 2023-12-08 8:41 UTC (permalink / raw)
To: Roger Quadros
Cc: dario.binacchi, hs, enrico.leto, trini, praneeth, nm, vigneshr,
u-boot
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
--
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________
Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: nand: omap_gpmc: Fix NAND in SPL for AM335x
2023-12-08 8:41 ` Michael Nazzareno Trimarchi
@ 2023-12-08 8:44 ` Roger Quadros
0 siblings, 0 replies; 14+ messages in thread
From: Roger Quadros @ 2023-12-08 8:44 UTC (permalink / raw)
To: Michael Nazzareno Trimarchi
Cc: dario.binacchi, hs, enrico.leto, trini, praneeth, nm, vigneshr,
u-boot
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
>
>
>
--
cheers,
-roger
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: nand: omap_gpmc: Fix NAND in SPL for AM335x
2023-11-25 11:16 [PATCH] mtd: nand: omap_gpmc: Fix NAND in SPL for AM335x Roger Quadros
` (3 preceding siblings ...)
2023-12-08 8:31 ` Roger Quadros
@ 2023-12-08 16:22 ` Michael Nazzareno Trimarchi
4 siblings, 0 replies; 14+ messages in thread
From: Michael Nazzareno Trimarchi @ 2023-12-08 16:22 UTC (permalink / raw)
To: Roger Quadros
Cc: dario.binacchi, hs, enrico.leto, trini, praneeth, nm, vigneshr,
u-boot
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
>
--
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________
Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: nand: omap_gpmc: Fix NAND in SPL for AM335x
2023-12-08 8:31 ` Roger Quadros
2023-12-08 8:41 ` Michael Nazzareno Trimarchi
@ 2023-12-09 19:50 ` Tom Rini
1 sibling, 0 replies; 14+ messages in thread
From: Tom Rini @ 2023-12-09 19:50 UTC (permalink / raw)
To: Roger Quadros
Cc: dario.binacchi, michael, hs, enrico.leto, praneeth, nm, vigneshr,
u-boot
[-- Attachment #1: Type: text/plain, Size: 1236 bytes --]
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 :)
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: nand: omap_gpmc: Fix NAND in SPL for AM335x
2023-12-07 14:22 ` Roger Quadros
@ 2023-12-09 19:51 ` Tom Rini
0 siblings, 0 replies; 14+ messages in thread
From: Tom Rini @ 2023-12-09 19:51 UTC (permalink / raw)
To: Roger Quadros
Cc: dario.binacchi, michael, hs, enrico.leto, praneeth, nm, vigneshr,
u-boot
[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]
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.
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-12-09 19:51 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-25 11:16 [PATCH] mtd: nand: omap_gpmc: Fix NAND in SPL for AM335x Roger Quadros
2023-11-25 13:06 ` Michael Nazzareno Trimarchi
2023-11-27 14:00 ` Leto, Enrico
2023-11-27 14:43 ` Michael Nazzareno Trimarchi
2023-11-28 9:21 ` Roger Quadros
2023-11-26 17:35 ` Tom Rini
2023-12-07 14:22 ` Roger Quadros
2023-12-09 19:51 ` Tom Rini
2023-11-27 13:39 ` Heiko Schocher
2023-12-08 8:31 ` Roger Quadros
2023-12-08 8:41 ` Michael Nazzareno Trimarchi
2023-12-08 8:44 ` Roger Quadros
2023-12-09 19:50 ` Tom Rini
2023-12-08 16:22 ` Michael Nazzareno Trimarchi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.