GPMI NAND Regression on i.MX6S
Fabio Estevam
festevam at gmail.com
Wed Mar 16 15:08:46 CET 2022
Adding Han Xu's NXP email on Cc.
On Mon, Mar 14, 2022 at 10:31 AM Frieder Schrempf
<frieder.schrempf at kontron.de> wrote:
>
> Hello everyone,
>
> sorry to dig out an old thread, but the below patch which was applied
> upstream as 616f03dabacb causes a regression for me when trying to
> attach an UBI volume with U-Boot 2022.01 on a board with i.MX6 Solo and
> AMD/Spansion parallel NAND.
>
> The failure looks like this:
>
> ubi0: attaching mtd2
> ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
> from PEB 0:0, read 64 bytes
> ubi0 error: ubi_io_read: error -74 (ECC error) while reading 2048 bytes
> from PEB 0:2048, read 2048 bytes
> ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
> from PEB 1:0, read 64 bytes
> ubi0 error: ubi_io_read: error -74 (ECC error) while reading 2048 bytes
> from PEB 1:2048, read 2048 bytes
>
> The NAND as reported by Linux is:
>
> nand: device found, Manufacturer ID: 0x01, Chip ID: 0xdc
> nand: AMD/Spansion S34ML04G1
> nand: 512 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
>
> A different revision of the same board with a different NAND from
> manufacturer ESMT doesn't show the issue:
>
> nand: device found, Manufacturer ID: 0xc8, Chip ID: 0xdc
> nand: ESMT NAND 512MiB 3,3V 8-bit
> nand: 512 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
>
> When I revert the mentioned commit (see patch here: [1]), the UBI boot
> starts working again.
>
> Does anyone know what the problem is and how to properly solve it?
>
> Thanks for any help!
> Frieder
>
> [1]
> https://zerobin.net/?57a57a322bbdcf3c#rZa3vHlWi+RxtRomoljtrngqWwiv6v4Js/2LNfdV10o=
>
> Am 04.05.20 um 16:08 schrieb Peng Fan:
> > From: Ye Li <ye.li at nxp.com>
> >
> > The code change updated the NAND driver BCH ECC layout algorithm to
> > support large oob size NAND chips(oob > 1024 bytes) and proposed a new
> > way to set ECC layout.
> >
> > Current implementation requires each chunk size larger than oob size so
> > the bad block marker (BBM) can be guaranteed located in data chunk. The
> > ECC layout always using the unbalanced layout(Ecc for both meta and
> > Data0 chunk), but for the NAND chips with oob larger than 1k, the driver
> > cannot support because BCH doesn’t support GF 15 for 2K chunk.
> >
> > The change keeps the data chunk no larger than 1k and adjust the ECC
> > strength or ECC layout to locate the BBM in data chunk. General idea for
> > large oob NAND chips is
> >
> > 1.Try all ECC strength from the minimum value required by NAND spec to
> > the maximum one that works, any ECC makes the BBM locate in data chunk
> > can be chosen.
> >
> > 2.If none of them works, using separate ECC for meta, which will add one
> > extra ecc with the same ECC strength as other data chunks. This extra
> > ECC can guarantee BBM located in data chunk, of course, we need to check
> > if oob can afford it.
> >
> > Previous code has two methods for ECC layout setting, the
> > legacy_calc_ecc_layout and calc_ecc_layout_by_info, the difference
> > between these two methods is, legacy_calc_ecc_layout set the chunk size
> > larger chan oob size and then set the maximum ECC strength that oob can
> > afford. While the calc_ecc_layout_by_info set chunk size and ECC
> > strength according to NAND spec. It has been proved that the first
> > method cannot provide safe ECC strength for some modern NAND chips, so
> > in current code,
> >
> > 1. Driver read NAND parameters first and then chose the proper ECC
> > layout setting method.
> >
> > 2. If the oob is large or NAND required data chunk larger than oob size,
> > chose calc_ecc_for_large_oob, otherwise use calc_ecc_layout_by_info
> >
> > 3. legacy_calc_ecc_layout only used for some NAND chips does not contains
> > necessary information. So this is only a backup plan, it is NOT
> > recommended to use these NAND chips.
> >
> > Signed-off-by: Han Xu <b45815 at freescale.com>
> > Signed-off-by: Ye Li <ye.li at nxp.com>
> > Signed-off-by: Peng Fan <peng.fan at nxp.com>
> > ---
> > drivers/mtd/nand/raw/mxs_nand.c | 205 ++++++++++++++++++++++++++--------------
> > include/mxs_nand.h | 12 ++-
> > 2 files changed, 144 insertions(+), 73 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/mxs_nand.c b/drivers/mtd/nand/raw/mxs_nand.c
> > index fe8097c146..8da59e39c0 100644
> > --- a/drivers/mtd/nand/raw/mxs_nand.c
> > +++ b/drivers/mtd/nand/raw/mxs_nand.c
> > @@ -112,53 +112,32 @@ static uint32_t mxs_nand_aux_status_offset(void)
> > return (MXS_NAND_METADATA_SIZE + 0x3) & ~0x3;
> > }
> >
> > -static inline int mxs_nand_calc_mark_offset(struct bch_geometry *geo,
> > - uint32_t page_data_size)
> > +static inline bool mxs_nand_bbm_in_data_chunk(struct bch_geometry *geo, struct mtd_info *mtd,
> > + unsigned int *chunk_num)
> > {
> > - uint32_t chunk_data_size_in_bits = geo->ecc_chunk_size * 8;
> > - uint32_t chunk_ecc_size_in_bits = geo->ecc_strength * geo->gf_len;
> > - uint32_t chunk_total_size_in_bits;
> > - uint32_t block_mark_chunk_number;
> > - uint32_t block_mark_chunk_bit_offset;
> > - uint32_t block_mark_bit_offset;
> > + unsigned int i, j;
> >
> > - chunk_total_size_in_bits =
> > - chunk_data_size_in_bits + chunk_ecc_size_in_bits;
> > -
> > - /* Compute the bit offset of the block mark within the physical page. */
> > - block_mark_bit_offset = page_data_size * 8;
> > -
> > - /* Subtract the metadata bits. */
> > - block_mark_bit_offset -= MXS_NAND_METADATA_SIZE * 8;
> > -
> > - /*
> > - * Compute the chunk number (starting at zero) in which the block mark
> > - * appears.
> > - */
> > - block_mark_chunk_number =
> > - block_mark_bit_offset / chunk_total_size_in_bits;
> > -
> > - /*
> > - * Compute the bit offset of the block mark within its chunk, and
> > - * validate it.
> > - */
> > - block_mark_chunk_bit_offset = block_mark_bit_offset -
> > - (block_mark_chunk_number * chunk_total_size_in_bits);
> > + if (geo->ecc_chunk0_size != geo->ecc_chunkn_size) {
> > + dev_err(this->dev, "The size of chunk0 must equal to chunkn\n");
> > + return false;
> > + }
> >
> > - if (block_mark_chunk_bit_offset > chunk_data_size_in_bits)
> > - return -EINVAL;
> > + i = (mtd->writesize * 8 - MXS_NAND_METADATA_SIZE * 8) /
> > + (geo->gf_len * geo->ecc_strength +
> > + geo->ecc_chunkn_size * 8);
> >
> > - /*
> > - * Now that we know the chunk number in which the block mark appears,
> > - * we can subtract all the ECC bits that appear before it.
> > - */
> > - block_mark_bit_offset -=
> > - block_mark_chunk_number * chunk_ecc_size_in_bits;
> > + j = (mtd->writesize * 8 - MXS_NAND_METADATA_SIZE * 8) -
> > + (geo->gf_len * geo->ecc_strength +
> > + geo->ecc_chunkn_size * 8) * i;
> >
> > - geo->block_mark_byte_offset = block_mark_bit_offset >> 3;
> > - geo->block_mark_bit_offset = block_mark_bit_offset & 0x7;
> > + if (j < geo->ecc_chunkn_size * 8) {
> > + *chunk_num = i + 1;
> > + dev_dbg(this->dev, "Set ecc to %d and bbm in chunk %d\n",
> > + geo->ecc_strength, *chunk_num);
> > + return true;
> > + }
> >
> > - return 0;
> > + return false;
> > }
> >
> > static inline int mxs_nand_calc_ecc_layout_by_info(struct bch_geometry *geo,
> > @@ -168,6 +147,7 @@ static inline int mxs_nand_calc_ecc_layout_by_info(struct bch_geometry *geo,
> > {
> > struct nand_chip *chip = mtd_to_nand(mtd);
> > struct mxs_nand_info *nand_info = nand_get_controller_data(chip);
> > + unsigned int block_mark_bit_offset;
> >
> > switch (ecc_step) {
> > case SZ_512:
> > @@ -180,45 +160,51 @@ static inline int mxs_nand_calc_ecc_layout_by_info(struct bch_geometry *geo,
> > return -EINVAL;
> > }
> >
> > - geo->ecc_chunk_size = ecc_step;
> > + geo->ecc_chunk0_size = ecc_step;
> > + geo->ecc_chunkn_size = ecc_step;
> > geo->ecc_strength = round_up(ecc_strength, 2);
> >
> > /* Keep the C >= O */
> > - if (geo->ecc_chunk_size < mtd->oobsize)
> > + if (geo->ecc_chunkn_size < mtd->oobsize)
> > return -EINVAL;
> >
> > if (geo->ecc_strength > nand_info->max_ecc_strength_supported)
> > return -EINVAL;
> >
> > - geo->ecc_chunk_count = mtd->writesize / geo->ecc_chunk_size;
> > + geo->ecc_chunk_count = mtd->writesize / geo->ecc_chunkn_size;
> > +
> > + /* For bit swap. */
> > + block_mark_bit_offset = mtd->writesize * 8 -
> > + (geo->ecc_strength * geo->gf_len * (geo->ecc_chunk_count - 1)
> > + + MXS_NAND_METADATA_SIZE * 8);
> > +
> > + geo->block_mark_byte_offset = block_mark_bit_offset / 8;
> > + geo->block_mark_bit_offset = block_mark_bit_offset % 8;
> >
> > return 0;
> > }
> >
> > -static inline int mxs_nand_calc_ecc_layout(struct bch_geometry *geo,
> > +static inline int mxs_nand_legacy_calc_ecc_layout(struct bch_geometry *geo,
> > struct mtd_info *mtd)
> > {
> > struct nand_chip *chip = mtd_to_nand(mtd);
> > struct mxs_nand_info *nand_info = nand_get_controller_data(chip);
> > + unsigned int block_mark_bit_offset;
> >
> > /* The default for the length of Galois Field. */
> > geo->gf_len = 13;
> >
> > /* The default for chunk size. */
> > - geo->ecc_chunk_size = 512;
> > + geo->ecc_chunk0_size = 512;
> > + geo->ecc_chunkn_size = 512;
> >
> > - if (geo->ecc_chunk_size < mtd->oobsize) {
> > + if (geo->ecc_chunkn_size < mtd->oobsize) {
> > geo->gf_len = 14;
> > - geo->ecc_chunk_size *= 2;
> > + geo->ecc_chunk0_size *= 2;
> > + geo->ecc_chunkn_size *= 2;
> > }
> >
> > - if (mtd->oobsize > geo->ecc_chunk_size) {
> > - printf("Not support the NAND chips whose oob size is larger then %d bytes!\n",
> > - geo->ecc_chunk_size);
> > - return -EINVAL;
> > - }
> > -
> > - geo->ecc_chunk_count = mtd->writesize / geo->ecc_chunk_size;
> > + geo->ecc_chunk_count = mtd->writesize / geo->ecc_chunkn_size;
> >
> > /*
> > * Determine the ECC layout with the formula:
> > @@ -234,6 +220,84 @@ static inline int mxs_nand_calc_ecc_layout(struct bch_geometry *geo,
> > geo->ecc_strength = min(round_down(geo->ecc_strength, 2),
> > nand_info->max_ecc_strength_supported);
> >
> > + block_mark_bit_offset = mtd->writesize * 8 -
> > + (geo->ecc_strength * geo->gf_len * (geo->ecc_chunk_count - 1)
> > + + MXS_NAND_METADATA_SIZE * 8);
> > +
> > + geo->block_mark_byte_offset = block_mark_bit_offset / 8;
> > + geo->block_mark_bit_offset = block_mark_bit_offset % 8;
> > +
> > + return 0;
> > +}
> > +
> > +static inline int mxs_nand_calc_ecc_for_large_oob(struct bch_geometry *geo,
> > + struct mtd_info *mtd)
> > +{
> > + struct nand_chip *chip = mtd_to_nand(mtd);
> > + struct mxs_nand_info *nand_info = nand_get_controller_data(chip);
> > + unsigned int block_mark_bit_offset;
> > + unsigned int max_ecc;
> > + unsigned int bbm_chunk;
> > + unsigned int i;
> > +
> > + /* sanity check for the minimum ecc nand required */
> > + if (!(chip->ecc_strength_ds > 0 && chip->ecc_step_ds > 0))
> > + return -EINVAL;
> > + geo->ecc_strength = chip->ecc_strength_ds;
> > +
> > + /* calculate the maximum ecc platform can support*/
> > + geo->gf_len = 14;
> > + geo->ecc_chunk0_size = 1024;
> > + geo->ecc_chunkn_size = 1024;
> > + geo->ecc_chunk_count = mtd->writesize / geo->ecc_chunkn_size;
> > + max_ecc = ((mtd->oobsize - MXS_NAND_METADATA_SIZE) * 8)
> > + / (geo->gf_len * geo->ecc_chunk_count);
> > + max_ecc = min(round_down(max_ecc, 2),
> > + nand_info->max_ecc_strength_supported);
> > +
> > +
> > + /* search a supported ecc strength that makes bbm */
> > + /* located in data chunk */
> > + geo->ecc_strength = chip->ecc_strength_ds;
> > + while (!(geo->ecc_strength > max_ecc)) {
> > + if (mxs_nand_bbm_in_data_chunk(geo, mtd, &bbm_chunk))
> > + break;
> > + geo->ecc_strength += 2;
> > + }
> > +
> > + /* if none of them works, keep using the minimum ecc */
> > + /* nand required but changing ecc page layout */
> > + if (geo->ecc_strength > max_ecc) {
> > + geo->ecc_strength = chip->ecc_strength_ds;
> > + /* add extra ecc for meta data */
> > + geo->ecc_chunk0_size = 0;
> > + geo->ecc_chunk_count = (mtd->writesize / geo->ecc_chunkn_size) + 1;
> > + geo->ecc_for_meta = 1;
> > + /* check if oob can afford this extra ecc chunk */
> > + if (mtd->oobsize * 8 < MXS_NAND_METADATA_SIZE * 8 +
> > + geo->gf_len * geo->ecc_strength
> > + * geo->ecc_chunk_count) {
> > + printf("unsupported NAND chip with new layout\n");
> > + return -EINVAL;
> > + }
> > +
> > + /* calculate in which chunk bbm located */
> > + bbm_chunk = (mtd->writesize * 8 - MXS_NAND_METADATA_SIZE * 8 -
> > + geo->gf_len * geo->ecc_strength) /
> > + (geo->gf_len * geo->ecc_strength +
> > + geo->ecc_chunkn_size * 8) + 1;
> > + }
> > +
> > + /* calculate the number of ecc chunk behind the bbm */
> > + i = (mtd->writesize / geo->ecc_chunkn_size) - bbm_chunk + 1;
> > +
> > + block_mark_bit_offset = mtd->writesize * 8 -
> > + (geo->ecc_strength * geo->gf_len * (geo->ecc_chunk_count - i)
> > + + MXS_NAND_METADATA_SIZE * 8);
> > +
> > + geo->block_mark_byte_offset = block_mark_bit_offset / 8;
> > + geo->block_mark_bit_offset = block_mark_bit_offset % 8;
> > +
> > return 0;
> > }
> >
> > @@ -983,18 +1047,23 @@ static int mxs_nand_set_geometry(struct mtd_info *mtd, struct bch_geometry *geo)
> > struct nand_chip *nand = mtd_to_nand(mtd);
> > struct mxs_nand_info *nand_info = nand_get_controller_data(nand);
> >
> > - if (chip->ecc.strength > 0 && chip->ecc.size > 0)
> > - return mxs_nand_calc_ecc_layout_by_info(geo, mtd,
> > - chip->ecc.strength, chip->ecc.size);
> > + if (chip->ecc_strength_ds > nand_info->max_ecc_strength_supported) {
> > + printf("unsupported NAND chip, minimum ecc required %d\n"
> > + , chip->ecc_strength_ds);
> > + return -EINVAL;
> > + }
> > +
> > + if (!(chip->ecc_strength_ds > 0 && chip->ecc_step_ds > 0) &&
> > + (mtd->oobsize < 1024)) {
> > + dev_warn(this->dev, "use legacy bch geometry\n");
> > + return mxs_nand_legacy_calc_ecc_layout(geo, mtd);
> > + }
> >
> > - if (nand_info->use_minimum_ecc ||
> > - mxs_nand_calc_ecc_layout(geo, mtd)) {
> > - if (!(chip->ecc_strength_ds > 0 && chip->ecc_step_ds > 0))
> > - return -EINVAL;
> > + if (mtd->oobsize > 1024 || chip->ecc_step_ds < mtd->oobsize)
> > + return mxs_nand_calc_ecc_for_large_oob(geo, mtd);
> >
> > - return mxs_nand_calc_ecc_layout_by_info(geo, mtd,
> > + return mxs_nand_calc_ecc_layout_by_info(geo, mtd,
> > chip->ecc_strength_ds, chip->ecc_step_ds);
> > - }
> >
> > return 0;
> > }
> > @@ -1025,8 +1094,6 @@ int mxs_nand_setup_ecc(struct mtd_info *mtd)
> > if (ret)
> > return ret;
> >
> > - mxs_nand_calc_mark_offset(geo, mtd->writesize);
> > -
> > /* Configure BCH and set NFC geometry */
> > mxs_reset_block(&bch_regs->hw_bch_ctrl_reg);
> >
> > @@ -1034,7 +1101,7 @@ int mxs_nand_setup_ecc(struct mtd_info *mtd)
> > tmp = (geo->ecc_chunk_count - 1) << BCH_FLASHLAYOUT0_NBLOCKS_OFFSET;
> > tmp |= MXS_NAND_METADATA_SIZE << BCH_FLASHLAYOUT0_META_SIZE_OFFSET;
> > tmp |= (geo->ecc_strength >> 1) << BCH_FLASHLAYOUT0_ECC0_OFFSET;
> > - tmp |= geo->ecc_chunk_size >> MXS_NAND_CHUNK_DATA_CHUNK_SIZE_SHIFT;
> > + tmp |= geo->ecc_chunk0_size >> MXS_NAND_CHUNK_DATA_CHUNK_SIZE_SHIFT;
> > tmp |= (geo->gf_len == 14 ? 1 : 0) <<
> > BCH_FLASHLAYOUT0_GF13_0_GF14_1_OFFSET;
> > writel(tmp, &bch_regs->hw_bch_flash0layout0);
> > @@ -1043,7 +1110,7 @@ int mxs_nand_setup_ecc(struct mtd_info *mtd)
> > tmp = (mtd->writesize + mtd->oobsize)
> > << BCH_FLASHLAYOUT1_PAGE_SIZE_OFFSET;
> > tmp |= (geo->ecc_strength >> 1) << BCH_FLASHLAYOUT1_ECCN_OFFSET;
> > - tmp |= geo->ecc_chunk_size >> MXS_NAND_CHUNK_DATA_CHUNK_SIZE_SHIFT;
> > + tmp |= geo->ecc_chunkn_size >> MXS_NAND_CHUNK_DATA_CHUNK_SIZE_SHIFT;
> > tmp |= (geo->gf_len == 14 ? 1 : 0) <<
> > BCH_FLASHLAYOUT1_GF13_0_GF14_1_OFFSET;
> > writel(tmp, &bch_regs->hw_bch_flash0layout1);
> > @@ -1268,7 +1335,7 @@ int mxs_nand_init_ctrl(struct mxs_nand_info *nand_info)
> >
> > nand->ecc.layout = &fake_ecc_layout;
> > nand->ecc.mode = NAND_ECC_HW;
> > - nand->ecc.size = nand_info->bch_geometry.ecc_chunk_size;
> > + nand->ecc.size = nand_info->bch_geometry.ecc_chunkn_size;
> > nand->ecc.strength = nand_info->bch_geometry.ecc_strength;
> >
> > /* second phase scan */
> > diff --git a/include/mxs_nand.h b/include/mxs_nand.h
> > index ada20483d0..497da77a16 100644
> > --- a/include/mxs_nand.h
> > +++ b/include/mxs_nand.h
> > @@ -16,22 +16,26 @@
> > * @gf_len: The length of Galois Field. (e.g., 13 or 14)
> > * @ecc_strength: A number that describes the strength of the ECC
> > * algorithm.
> > - * @ecc_chunk_size: The size, in bytes, of a single ECC chunk. Note
> > - * the first chunk in the page includes both data and
> > - * metadata, so it's a bit larger than this value.
> > + * @ecc_chunk0_size: The size, in bytes, of a first ECC chunk.
> > + * @ecc_chunkn_size: The size, in bytes, of a single ECC chunk after
> > + * the first chunk in the page.
> > * @ecc_chunk_count: The number of ECC chunks in the page,
> > * @block_mark_byte_offset: The byte offset in the ECC-based page view at
> > * which the underlying physical block mark appears.
> > * @block_mark_bit_offset: The bit offset into the ECC-based page view at
> > * which the underlying physical block mark appears.
> > + * @ecc_for_meta: The flag to indicate if there is a dedicate ecc
> > + * for meta.
> > */
> > struct bch_geometry {
> > unsigned int gf_len;
> > unsigned int ecc_strength;
> > - unsigned int ecc_chunk_size;
> > + unsigned int ecc_chunk0_size;
> > + unsigned int ecc_chunkn_size;
> > unsigned int ecc_chunk_count;
> > unsigned int block_mark_byte_offset;
> > unsigned int block_mark_bit_offset;
> > + unsigned int ecc_for_meta; /* ECC for meta data */
> > };
> >
> > struct mxs_nand_info {
> >
More information about the U-Boot
mailing list