All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] mtd: gpmi: fix the bch setting backward compatible issue
@ 2022-03-24 21:38 Han Xu
  2022-03-24 21:43 ` Fabio Estevam
  0 siblings, 1 reply; 2+ messages in thread
From: Han Xu @ 2022-03-24 21:38 UTC (permalink / raw)
  To: sbabic, sean, frieder.schrempf, festevam
  Cc: ye.li, peng.fan, u-boot, han.xu, miquel.raynal, uboot-imx

Previous u-boot code changed the default bch setting behavior and caused
backward compatible issue. This fix choose the legacy bch geometry back
again as the default option. If the minimum ecc strength that NAND chips
required need to be chosen, it can be enabled by either adding DT flag
"fsl,use-minimum-ecc" or CONFIG_NAND_MXS_USE_MINIMUM_ECC in configs. The
unused flag "fsl,legacy-bch-geometry" get removed.

Fixes: 51cdf83eea (mtd: gpmi: provide the option to use legacy bch geometry)
Fixes: 616f03daba (mtd: gpmi: change the BCH layout setting for large
oob NAND)
Tested-by: Tim Harvey <tharvey@gateworks.com>
Tested-by: Sean Nyekjaer <sean@geanix.com>
Signed-off-by: Han Xu <han.xu@nxp.com>

---
Changes in v2:
 - change the commit log about backward compatible issue is in u-boot
 - removed the unused flag
 - add fixes and test tag

Changes in v3:
 - fix the fixes syntax
 - fix the change log syntax
 - add more test tag
---
 drivers/mtd/nand/raw/mxs_nand.c    | 71 ++++++++++++++++++++++++++----
 drivers/mtd/nand/raw/mxs_nand_dt.c |  2 -
 include/mxs_nand.h                 |  2 -
 3 files changed, 62 insertions(+), 13 deletions(-)

diff --git a/drivers/mtd/nand/raw/mxs_nand.c b/drivers/mtd/nand/raw/mxs_nand.c
index 748056a43e..ee5d7fde9c 100644
--- a/drivers/mtd/nand/raw/mxs_nand.c
+++ b/drivers/mtd/nand/raw/mxs_nand.c
@@ -195,6 +195,7 @@ static inline int mxs_nand_legacy_calc_ecc_layout(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;
+	int corr, ds_corr;
 
 	/* The default for the length of Galois Field. */
 	geo->gf_len = 13;
@@ -225,6 +226,17 @@ static inline int mxs_nand_legacy_calc_ecc_layout(struct bch_geometry *geo,
 	geo->ecc_strength = min(round_down(geo->ecc_strength, 2),
 				nand_info->max_ecc_strength_supported);
 
+	/* check ecc strength, same as nand_ecc_is_strong_enough() did*/
+	if (chip->ecc_step_ds) {
+		corr = mtd->writesize * geo->ecc_strength /
+		       geo->ecc_chunkn_size;
+		ds_corr = mtd->writesize * chip->ecc_strength_ds /
+		       chip->ecc_step_ds;
+		if (corr < ds_corr ||
+		    geo->ecc_strength < chip->ecc_strength_ds)
+			return -EINVAL;
+	}
+
 	block_mark_bit_offset = mtd->writesize * 8 -
 		(geo->ecc_strength * geo->gf_len * (geo->ecc_chunk_count - 1)
 				+ MXS_NAND_METADATA_SIZE * 8);
@@ -1111,6 +1123,7 @@ static int mxs_nand_set_geometry(struct mtd_info *mtd, struct bch_geometry *geo)
 	struct nand_chip *chip = mtd_to_nand(mtd);
 	struct nand_chip *nand = mtd_to_nand(mtd);
 	struct mxs_nand_info *nand_info = nand_get_controller_data(nand);
+	int err;
 
 	if (chip->ecc_strength_ds > nand_info->max_ecc_strength_supported) {
 		printf("unsupported NAND chip, minimum ecc required %d\n"
@@ -1118,19 +1131,57 @@ static int mxs_nand_set_geometry(struct mtd_info *mtd, struct bch_geometry *geo)
 		return -EINVAL;
 	}
 
-	if ((!(chip->ecc_strength_ds > 0 && chip->ecc_step_ds > 0) &&
-	     mtd->oobsize < 1024) || nand_info->legacy_bch_geometry) {
-		dev_warn(mtd->dev, "use legacy bch geometry\n");
-		return mxs_nand_legacy_calc_ecc_layout(geo, mtd);
+	/* use the legacy bch setting by default */
+	if ((!nand_info->use_minimum_ecc && mtd->oobsize < 1024) ||
+	    !(chip->ecc_strength_ds > 0 && chip->ecc_step_ds > 0)) {
+		dev_dbg(mtd->dev, "use legacy bch geometry\n");
+		err = mxs_nand_legacy_calc_ecc_layout(geo, mtd);
+		if (!err)
+			return 0;
 	}
 
-	if (mtd->oobsize > 1024 || chip->ecc_step_ds < mtd->oobsize)
-		return mxs_nand_calc_ecc_for_large_oob(geo, mtd);
+	/* for large oob nand */
+	if (mtd->oobsize > 1024) {
+		dev_dbg(mtd->dev, "use large oob bch geometry\n");
+		err = mxs_nand_calc_ecc_for_large_oob(geo, mtd);
+		if (!err)
+			return 0;
+	}
 
-	return mxs_nand_calc_ecc_layout_by_info(geo, mtd,
-				chip->ecc_strength_ds, chip->ecc_step_ds);
+	/* otherwise use the minimum ecc nand chips required */
+	dev_dbg(mtd->dev, "use minimum ecc bch geometry\n");
+	err = mxs_nand_calc_ecc_layout_by_info(geo, mtd, chip->ecc_strength_ds,
+					       chip->ecc_step_ds);
 
-	return 0;
+	if (err)
+		dev_err(mtd->dev, "none of the bch geometry setting works\n");
+
+	return err;
+}
+
+void mxs_nand_dump_geo(struct mtd_info *mtd)
+{
+	struct nand_chip *nand = mtd_to_nand(mtd);
+	struct mxs_nand_info *nand_info = nand_get_controller_data(nand);
+	struct bch_geometry *geo = &nand_info->bch_geometry;
+
+	dev_dbg(mtd->dev, "BCH Geometry :\n"
+		"GF Length\t\t: %u\n"
+		"ECC Strength\t\t: %u\n"
+		"ECC for Meta\t\t: %u\n"
+		"ECC Chunk0 Size\t\t: %u\n"
+		"ECC Chunkn Size\t\t: %u\n"
+		"ECC Chunk Count\t\t: %u\n"
+		"Block Mark Byte Offset\t: %u\n"
+		"Block Mark Bit Offset\t: %u\n",
+		geo->gf_len,
+		geo->ecc_strength,
+		geo->ecc_for_meta,
+		geo->ecc_chunk0_size,
+		geo->ecc_chunkn_size,
+		geo->ecc_chunk_count,
+		geo->block_mark_byte_offset,
+		geo->block_mark_bit_offset);
 }
 
 /*
@@ -1159,6 +1210,8 @@ int mxs_nand_setup_ecc(struct mtd_info *mtd)
 	if (ret)
 		return ret;
 
+	mxs_nand_dump_geo(mtd);
+
 	/* Configure BCH and set NFC geometry */
 	mxs_reset_block(&bch_regs->hw_bch_ctrl_reg);
 
diff --git a/drivers/mtd/nand/raw/mxs_nand_dt.c b/drivers/mtd/nand/raw/mxs_nand_dt.c
index 878796d555..b9833a646f 100644
--- a/drivers/mtd/nand/raw/mxs_nand_dt.c
+++ b/drivers/mtd/nand/raw/mxs_nand_dt.c
@@ -92,8 +92,6 @@ static int mxs_nand_dt_probe(struct udevice *dev)
 
 	info->use_minimum_ecc = dev_read_bool(dev, "fsl,use-minimum-ecc");
 
-	info->legacy_bch_geometry = dev_read_bool(dev, "fsl,legacy-bch-geometry");
-
 	if (IS_ENABLED(CONFIG_CLK) && IS_ENABLED(CONFIG_IMX8)) {
 		/* Assigned clock already set clock */
 		struct clk gpmi_clk;
diff --git a/include/mxs_nand.h b/include/mxs_nand.h
index 66c909318d..741dc8734e 100644
--- a/include/mxs_nand.h
+++ b/include/mxs_nand.h
@@ -44,8 +44,6 @@ struct mxs_nand_info {
 	struct udevice *dev;
 	unsigned int	max_ecc_strength_supported;
 	bool		use_minimum_ecc;
-	/* legacy bch geometry flag */
-	bool		legacy_bch_geometry;
 	int		cur_chip;
 
 	uint32_t	cmd_queue_len;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 2+ messages in thread
* Re: [PATCH v3] mtd: gpmi: fix the bch setting backward compatible issue
  2022-03-24 21:38 [PATCH v3] mtd: gpmi: fix the bch setting backward compatible issue Han Xu
@ 2022-03-24 21:43 ` Fabio Estevam
  0 siblings, 0 replies; 2+ messages in thread
From: Fabio Estevam @ 2022-03-24 21:43 UTC (permalink / raw)
  To: Han Xu
  Cc: Stefano Babic, Sean Nyekjaer, Schrempf Frieder, Ye Li, Peng Fan,
	U-Boot-Denx, Miquel Raynal, dl-uboot-imx

On Thu, Mar 24, 2022 at 6:39 PM Han Xu <han.xu@nxp.com> wrote:
>
> Previous u-boot code changed the default bch setting behavior and caused
> backward compatible issue. This fix choose the legacy bch geometry back
> again as the default option. If the minimum ecc strength that NAND chips
> required need to be chosen, it can be enabled by either adding DT flag
> "fsl,use-minimum-ecc" or CONFIG_NAND_MXS_USE_MINIMUM_ECC in configs. The
> unused flag "fsl,legacy-bch-geometry" get removed.
>
> Fixes: 51cdf83eea (mtd: gpmi: provide the option to use legacy bch geometry)
> Fixes: 616f03daba (mtd: gpmi: change the BCH layout setting for large
> oob NAND)

Like I said previously, the recommended Fixes tag should contain
12-digit hash followed by ("text"),
so it should be:

Fixes: 51cdf83eea45 ("mtd: gpmi: provide the option to use legacy bch geometry")
Fixes: 616f03dabacb ("mtd: gpmi: change the BCH layout setting for
large oob NAND")

Please don't split the fixes tag in multiples lines.

> Tested-by: Tim Harvey <tharvey@gateworks.com>
> Tested-by: Sean Nyekjaer <sean@geanix.com>
> Signed-off-by: Han Xu <han.xu@nxp.com>
>
> ---
> Changes in v2:
>  - change the commit log about backward compatible issue is in u-boot
>  - removed the unused flag
>  - add fixes and test tag
>
> Changes in v3:
>  - fix the fixes syntax
>  - fix the change log syntax
>  - add more test tag
> ---
>  drivers/mtd/nand/raw/mxs_nand.c    | 71 ++++++++++++++++++++++++++----
>  drivers/mtd/nand/raw/mxs_nand_dt.c |  2 -
>  include/mxs_nand.h                 |  2 -
>  3 files changed, 62 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/mxs_nand.c b/drivers/mtd/nand/raw/mxs_nand.c
> index 748056a43e..ee5d7fde9c 100644
> --- a/drivers/mtd/nand/raw/mxs_nand.c
> +++ b/drivers/mtd/nand/raw/mxs_nand.c
> @@ -195,6 +195,7 @@ static inline int mxs_nand_legacy_calc_ecc_layout(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;
> +       int corr, ds_corr;
>
>         /* The default for the length of Galois Field. */
>         geo->gf_len = 13;
> @@ -225,6 +226,17 @@ static inline int mxs_nand_legacy_calc_ecc_layout(struct bch_geometry *geo,
>         geo->ecc_strength = min(round_down(geo->ecc_strength, 2),
>                                 nand_info->max_ecc_strength_supported);
>
> +       /* check ecc strength, same as nand_ecc_is_strong_enough() did*/
> +       if (chip->ecc_step_ds) {
> +               corr = mtd->writesize * geo->ecc_strength /
> +                      geo->ecc_chunkn_size;
> +               ds_corr = mtd->writesize * chip->ecc_strength_ds /
> +                      chip->ecc_step_ds;
> +               if (corr < ds_corr ||
> +                   geo->ecc_strength < chip->ecc_strength_ds)
> +                       return -EINVAL;
> +       }
> +
>         block_mark_bit_offset = mtd->writesize * 8 -
>                 (geo->ecc_strength * geo->gf_len * (geo->ecc_chunk_count - 1)
>                                 + MXS_NAND_METADATA_SIZE * 8);
> @@ -1111,6 +1123,7 @@ static int mxs_nand_set_geometry(struct mtd_info *mtd, struct bch_geometry *geo)
>         struct nand_chip *chip = mtd_to_nand(mtd);
>         struct nand_chip *nand = mtd_to_nand(mtd);
>         struct mxs_nand_info *nand_info = nand_get_controller_data(nand);
> +       int err;
>
>         if (chip->ecc_strength_ds > nand_info->max_ecc_strength_supported) {
>                 printf("unsupported NAND chip, minimum ecc required %d\n"
> @@ -1118,19 +1131,57 @@ static int mxs_nand_set_geometry(struct mtd_info *mtd, struct bch_geometry *geo)
>                 return -EINVAL;
>         }
>
> -       if ((!(chip->ecc_strength_ds > 0 && chip->ecc_step_ds > 0) &&
> -            mtd->oobsize < 1024) || nand_info->legacy_bch_geometry) {
> -               dev_warn(mtd->dev, "use legacy bch geometry\n");
> -               return mxs_nand_legacy_calc_ecc_layout(geo, mtd);
> +       /* use the legacy bch setting by default */
> +       if ((!nand_info->use_minimum_ecc && mtd->oobsize < 1024) ||
> +           !(chip->ecc_strength_ds > 0 && chip->ecc_step_ds > 0)) {
> +               dev_dbg(mtd->dev, "use legacy bch geometry\n");
> +               err = mxs_nand_legacy_calc_ecc_layout(geo, mtd);
> +               if (!err)
> +                       return 0;
>         }
>
> -       if (mtd->oobsize > 1024 || chip->ecc_step_ds < mtd->oobsize)
> -               return mxs_nand_calc_ecc_for_large_oob(geo, mtd);
> +       /* for large oob nand */
> +       if (mtd->oobsize > 1024) {
> +               dev_dbg(mtd->dev, "use large oob bch geometry\n");
> +               err = mxs_nand_calc_ecc_for_large_oob(geo, mtd);
> +               if (!err)
> +                       return 0;
> +       }
>
> -       return mxs_nand_calc_ecc_layout_by_info(geo, mtd,
> -                               chip->ecc_strength_ds, chip->ecc_step_ds);
> +       /* otherwise use the minimum ecc nand chips required */
> +       dev_dbg(mtd->dev, "use minimum ecc bch geometry\n");
> +       err = mxs_nand_calc_ecc_layout_by_info(geo, mtd, chip->ecc_strength_ds,
> +                                              chip->ecc_step_ds);
>
> -       return 0;
> +       if (err)
> +               dev_err(mtd->dev, "none of the bch geometry setting works\n");
> +
> +       return err;
> +}
> +
> +void mxs_nand_dump_geo(struct mtd_info *mtd)
> +{
> +       struct nand_chip *nand = mtd_to_nand(mtd);
> +       struct mxs_nand_info *nand_info = nand_get_controller_data(nand);
> +       struct bch_geometry *geo = &nand_info->bch_geometry;
> +
> +       dev_dbg(mtd->dev, "BCH Geometry :\n"
> +               "GF Length\t\t: %u\n"
> +               "ECC Strength\t\t: %u\n"
> +               "ECC for Meta\t\t: %u\n"
> +               "ECC Chunk0 Size\t\t: %u\n"
> +               "ECC Chunkn Size\t\t: %u\n"
> +               "ECC Chunk Count\t\t: %u\n"
> +               "Block Mark Byte Offset\t: %u\n"
> +               "Block Mark Bit Offset\t: %u\n",
> +               geo->gf_len,
> +               geo->ecc_strength,
> +               geo->ecc_for_meta,
> +               geo->ecc_chunk0_size,
> +               geo->ecc_chunkn_size,
> +               geo->ecc_chunk_count,
> +               geo->block_mark_byte_offset,
> +               geo->block_mark_bit_offset);
>  }
>
>  /*
> @@ -1159,6 +1210,8 @@ int mxs_nand_setup_ecc(struct mtd_info *mtd)
>         if (ret)
>                 return ret;
>
> +       mxs_nand_dump_geo(mtd);
> +
>         /* Configure BCH and set NFC geometry */
>         mxs_reset_block(&bch_regs->hw_bch_ctrl_reg);
>
> diff --git a/drivers/mtd/nand/raw/mxs_nand_dt.c b/drivers/mtd/nand/raw/mxs_nand_dt.c
> index 878796d555..b9833a646f 100644
> --- a/drivers/mtd/nand/raw/mxs_nand_dt.c
> +++ b/drivers/mtd/nand/raw/mxs_nand_dt.c
> @@ -92,8 +92,6 @@ static int mxs_nand_dt_probe(struct udevice *dev)
>
>         info->use_minimum_ecc = dev_read_bool(dev, "fsl,use-minimum-ecc");
>
> -       info->legacy_bch_geometry = dev_read_bool(dev, "fsl,legacy-bch-geometry");
> -
>         if (IS_ENABLED(CONFIG_CLK) && IS_ENABLED(CONFIG_IMX8)) {
>                 /* Assigned clock already set clock */
>                 struct clk gpmi_clk;
> diff --git a/include/mxs_nand.h b/include/mxs_nand.h
> index 66c909318d..741dc8734e 100644
> --- a/include/mxs_nand.h
> +++ b/include/mxs_nand.h
> @@ -44,8 +44,6 @@ struct mxs_nand_info {
>         struct udevice *dev;
>         unsigned int    max_ecc_strength_supported;
>         bool            use_minimum_ecc;
> -       /* legacy bch geometry flag */
> -       bool            legacy_bch_geometry;
>         int             cur_chip;
>
>         uint32_t        cmd_queue_len;
> --
> 2.17.1
>

^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-03-24 21:43 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-24 21:38 [PATCH v3] mtd: gpmi: fix the bch setting backward compatible issue Han Xu
2022-03-24 21:43 ` Fabio Estevam

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.