Re: [PATCH] mtd: nand: omap_gpmc: Fix NAND in SPL for AM335x

[Roger Quadros <rogerq@kernel.org>]

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