[U-Boot] [PATCH v3 0/5] mtd: nand: mxs_nand: improve ECC support

[U-Boot] [PATCH v3 0/5] mtd: nand: mxs_nand: improve ECC support

[Stefan Agner]

From: Stefan Agner <stefan.agner@toradex.com>

This patchset reworks the drivers ECC calculation to align more
with the Linux driver gpmi-nand.c. It aims to implements minimal
ECC support as supported by the NAND chip.

This is the rebased version after moving MXS NAND to Kconfig.

--
Stefan

Changes in v3:
- Fix indentation
- Rebased and removed already applied Kconfig change

Changes in v2:
- Extend the patchset with "report correct ECC parameters" patch

Stefan Agner (5):
  mtd: nand: mxs_nand: use self init
  mtd: nand: mxs_nand: allow to enable BBT support
  mtd: nand: mxs_nand: use structure for BCH geometry
  mtd: nand: mxs_nand: report correct ECC parameters
  mtd: nand: mxs_nand: add minimal ECC support

 drivers/mtd/nand/Kconfig    |   9 ++
 drivers/mtd/nand/mxs_nand.c | 282 ++++++++++++++++++++++--------------
 2 files changed, 180 insertions(+), 111 deletions(-)

-- 
2.17.0

[U-Boot] [PATCH v3 1/5] mtd: nand: mxs_nand: use self init

[Stefan Agner]

From: Stefan Agner <stefan.agner@toradex.com>

Instead of completing initialization via scan_bbt callback use
NAND self init to initialize the GPMI (MXS) NAND controller.

Suggested-by: Scott Wood <oss@buserror.net>
Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
---

Changes in v3:
- Fix indentation

Changes in v2: None

 drivers/mtd/nand/Kconfig    |  1 +
 drivers/mtd/nand/mxs_nand.c | 52 +++++++++++++++++++++----------------
 2 files changed, 30 insertions(+), 23 deletions(-)

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 94fbf89e4b..4db259fcb2 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -143,6 +143,7 @@ config NAND_MXC
 config NAND_MXS
 	bool "MXS NAND support"
 	depends on MX23 || MX28 || MX6 || MX7
+	select SYS_NAND_SELF_INIT
 	imply CMD_NAND
 	select APBH_DMA
 	select APBH_DMA_BURST if ARCH_MX6 || ARCH_MX7
diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c
index bed9b65ef4..9338f1889d 100644
--- a/drivers/mtd/nand/mxs_nand.c
+++ b/drivers/mtd/nand/mxs_nand.c
@@ -18,6 +18,7 @@
 #include <linux/mtd/rawnand.h>
 #include <linux/types.h>
 #include <malloc.h>
+#include <nand.h>
 #include <linux/errno.h>
 #include <asm/io.h>
 #include <asm/arch/clock.h>
@@ -47,6 +48,7 @@
 #define	MXS_NAND_BCH_TIMEOUT			10000
 
 struct mxs_nand_info {
+	struct nand_chip chip;
 	int		cur_chip;
 
 	uint32_t	cmd_queue_len;
@@ -972,20 +974,15 @@ static int mxs_nand_block_bad(struct mtd_info *mtd, loff_t ofs)
 }
 
 /*
- * Nominally, the purpose of this function is to look for or create the bad
- * block table. In fact, since the we call this function at the very end of
- * the initialization process started by nand_scan(), and we doesn't have a
- * more formal mechanism, we "hook" this function to continue init process.
- *
  * At this point, the physical NAND Flash chips have been identified and
  * counted, so we know the physical geometry. This enables us to make some
  * important configuration decisions.
  *
  * The return value of this function propagates directly back to this driver's
- * call to nand_scan(). Anything other than zero will cause this driver to
+ * board_nand_init(). Anything other than zero will cause this driver to
  * tear everything down and declare failure.
  */
-static int mxs_nand_scan_bbt(struct mtd_info *mtd)
+static int mxs_nand_setup_ecc(struct mtd_info *mtd)
 {
 	struct nand_chip *nand = mtd_to_nand(mtd);
 	struct mxs_nand_info *nand_info = nand_get_controller_data(nand);
@@ -1047,8 +1044,7 @@ static int mxs_nand_scan_bbt(struct mtd_info *mtd)
 		mtd->_block_markbad = mxs_nand_hook_block_markbad;
 	}
 
-	/* We use the reference implementation for bad block management. */
-	return nand_default_bbt(mtd);
+	return 0;
 }
 
 /*
@@ -1150,27 +1146,22 @@ err1:
 	return ret;
 }
 
-/*!
- * This function is called during the driver binding process.
- *
- * @param   pdev  the device structure used to store device specific
- *                information that is used by the suspend, resume and
- *                remove functions
- *
- * @return  The function always returns 0.
- */
-int board_nand_init(struct nand_chip *nand)
+void board_nand_init(void)
 {
+	struct mtd_info *mtd;
 	struct mxs_nand_info *nand_info;
+	struct nand_chip *nand;
 	int err;
 
 	nand_info = malloc(sizeof(struct mxs_nand_info));
 	if (!nand_info) {
 		printf("MXS NAND: Failed to allocate private data\n");
-		return -ENOMEM;
+			return;
 	}
 	memset(nand_info, 0, sizeof(struct mxs_nand_info));
 
+	nand = &nand_info->chip;
+	mtd = nand_to_mtd(nand);
 	err = mxs_nand_alloc_buffers(nand_info);
 	if (err)
 		goto err1;
@@ -1189,13 +1180,19 @@ int board_nand_init(struct nand_chip *nand)
 	nand->dev_ready		= mxs_nand_device_ready;
 	nand->select_chip	= mxs_nand_select_chip;
 	nand->block_bad		= mxs_nand_block_bad;
-	nand->scan_bbt		= mxs_nand_scan_bbt;
 
 	nand->read_byte		= mxs_nand_read_byte;
 
 	nand->read_buf		= mxs_nand_read_buf;
 	nand->write_buf		= mxs_nand_write_buf;
 
+	/* first scan to find the device and get the page size */
+	if (nand_scan_ident(mtd, CONFIG_SYS_MAX_NAND_DEVICE, NULL))
+		goto err2;
+
+	if (mxs_nand_setup_ecc(mtd))
+		goto err2;
+
 	nand->ecc.read_page	= mxs_nand_ecc_read_page;
 	nand->ecc.write_page	= mxs_nand_ecc_write_page;
 	nand->ecc.read_oob	= mxs_nand_ecc_read_oob;
@@ -1207,12 +1204,21 @@ int board_nand_init(struct nand_chip *nand)
 	nand->ecc.size		= 512;
 	nand->ecc.strength	= 8;
 
-	return 0;
+	/* second phase scan */
+	err = nand_scan_tail(mtd);
+	if (err)
+		goto err2;
+
+	err = nand_register(0, mtd);
+	if (err)
+		goto err2;
+
+	return;
 
 err2:
 	free(nand_info->data_buf);
 	free(nand_info->cmd_buf);
 err1:
 	free(nand_info);
-	return err;
+	return;
 }
-- 
2.17.0

[U-Boot] [PATCH v3 2/5] mtd: nand: mxs_nand: allow to enable BBT support

[Stefan Agner]

From: Stefan Agner <stefan.agner@toradex.com>

Add config option which allows to enable on flash bad block table
support. This has the same effect as when using the device tree
property "nand-on-flash-bbt" in Linux.

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
---

Changes in v3: None
Changes in v2: None

 drivers/mtd/nand/mxs_nand.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c
index 9338f1889d..867549e530 100644
--- a/drivers/mtd/nand/mxs_nand.c
+++ b/drivers/mtd/nand/mxs_nand.c
@@ -1172,6 +1172,10 @@ void board_nand_init(void)
 
 	memset(&fake_ecc_layout, 0, sizeof(fake_ecc_layout));
 
+#ifdef CONFIG_SYS_NAND_USE_FLASH_BBT
+	nand->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
+#endif
+
 	nand_set_controller_data(nand, nand_info);
 	nand->options |= NAND_NO_SUBPAGE_WRITE;
 
-- 
2.17.0

[U-Boot] [PATCH v3 3/5] mtd: nand: mxs_nand: use structure for BCH geometry

[Stefan Agner]

From: Stefan Agner <stefan.agner@toradex.com>

Calculate BCH geometry at start and store the information in
a structure. This avoids recalculation on every page access
and allows to calculate ECC relevant information in one place.
This patch does not change ECC layout or driver behavior in
any way.

The patch aligns the driver somewhat with the Linux GPMI NAND
driver which drives the same IP.

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
---

Changes in v3: None
Changes in v2: None

 drivers/mtd/nand/mxs_nand.c | 182 +++++++++++++++++++-----------------
 1 file changed, 95 insertions(+), 87 deletions(-)

diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c
index 867549e530..aa4c5a6526 100644
--- a/drivers/mtd/nand/mxs_nand.c
+++ b/drivers/mtd/nand/mxs_nand.c
@@ -30,7 +30,6 @@
 
 #define	MXS_NAND_DMA_DESCRIPTOR_COUNT		4
 
-#define	MXS_NAND_CHUNK_DATA_CHUNK_SIZE		512
 #if (defined(CONFIG_MX6) || defined(CONFIG_MX7))
 #define	MXS_NAND_CHUNK_DATA_CHUNK_SIZE_SHIFT	2
 #else
@@ -47,12 +46,35 @@
 
 #define	MXS_NAND_BCH_TIMEOUT			10000
 
+/**
+ * @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_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.
+ */
+struct bch_geometry {
+	unsigned int  gf_len;
+	unsigned int  ecc_strength;
+	unsigned int  ecc_chunk_size;
+	unsigned int  ecc_chunk_count;
+	unsigned int  block_mark_byte_offset;
+	unsigned int  block_mark_bit_offset;
+};
+
 struct mxs_nand_info {
 	struct nand_chip chip;
 	int		cur_chip;
 
 	uint32_t	cmd_queue_len;
 	uint32_t	data_buf_size;
+	struct bch_geometry bch_geometry;
 
 	uint8_t		*cmd_buf;
 	uint8_t		*data_buf;
@@ -75,8 +97,6 @@ struct mxs_nand_info {
 };
 
 struct nand_ecclayout fake_ecc_layout;
-static int chunk_data_size = MXS_NAND_CHUNK_DATA_CHUNK_SIZE;
-static int galois_field = 13;
 
 /*
  * Cache management functions
@@ -137,61 +157,21 @@ static void mxs_nand_return_dma_descs(struct mxs_nand_info *info)
 	info->desc_index = 0;
 }
 
-static uint32_t mxs_nand_ecc_chunk_cnt(uint32_t page_data_size)
-{
-	return page_data_size / chunk_data_size;
-}
-
-static uint32_t mxs_nand_ecc_size_in_bits(uint32_t ecc_strength)
-{
-	return ecc_strength * galois_field;
-}
-
 static uint32_t mxs_nand_aux_status_offset(void)
 {
 	return (MXS_NAND_METADATA_SIZE + 0x3) & ~0x3;
 }
 
-static inline uint32_t mxs_nand_get_ecc_strength(uint32_t page_data_size,
-						uint32_t page_oob_size)
+static inline int mxs_nand_calc_mark_offset(struct bch_geometry *geo,
+					    uint32_t page_data_size)
 {
-	int ecc_strength;
-	int max_ecc_strength_supported;
-
-	/* Refer to Chapter 17 for i.MX6DQ, Chapter 18 for i.MX6SX */
-	if (is_mx6sx() || is_mx7())
-		max_ecc_strength_supported = 62;
-	else
-		max_ecc_strength_supported = 40;
-
-	/*
-	 * Determine the ECC layout with the formula:
-	 *	ECC bits per chunk = (total page spare data bits) /
-	 *		(bits per ECC level) / (chunks per page)
-	 * where:
-	 *	total page spare data bits =
-	 *		(page oob size - meta data size) * (bits per byte)
-	 */
-	ecc_strength = ((page_oob_size - MXS_NAND_METADATA_SIZE) * 8)
-			/ (galois_field *
-			   mxs_nand_ecc_chunk_cnt(page_data_size));
-
-	return min(round_down(ecc_strength, 2), max_ecc_strength_supported);
-}
-
-static inline uint32_t mxs_nand_get_mark_offset(uint32_t page_data_size,
-						uint32_t ecc_strength)
-{
-	uint32_t chunk_data_size_in_bits;
-	uint32_t chunk_ecc_size_in_bits;
+	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;
 
-	chunk_data_size_in_bits = chunk_data_size * 8;
-	chunk_ecc_size_in_bits  = mxs_nand_ecc_size_in_bits(ecc_strength);
-
 	chunk_total_size_in_bits =
 			chunk_data_size_in_bits + chunk_ecc_size_in_bits;
 
@@ -216,7 +196,7 @@ static inline uint32_t mxs_nand_get_mark_offset(uint32_t page_data_size,
 			(block_mark_chunk_number * chunk_total_size_in_bits);
 
 	if (block_mark_chunk_bit_offset > chunk_data_size_in_bits)
-		return 1;
+		return -EINVAL;
 
 	/*
 	 * Now that we know the chunk number in which the block mark appears,
@@ -225,21 +205,59 @@ static inline uint32_t mxs_nand_get_mark_offset(uint32_t page_data_size,
 	block_mark_bit_offset -=
 		block_mark_chunk_number * chunk_ecc_size_in_bits;
 
-	return block_mark_bit_offset;
-}
+	geo->block_mark_byte_offset = block_mark_bit_offset >> 3;
+	geo->block_mark_bit_offset = block_mark_bit_offset & 0x7;
 
-static uint32_t mxs_nand_mark_byte_offset(struct mtd_info *mtd)
-{
-	uint32_t ecc_strength;
-	ecc_strength = mxs_nand_get_ecc_strength(mtd->writesize, mtd->oobsize);
-	return mxs_nand_get_mark_offset(mtd->writesize, ecc_strength) >> 3;
+	return 0;
 }
 
-static uint32_t mxs_nand_mark_bit_offset(struct mtd_info *mtd)
+static inline int mxs_nand_calc_ecc_layout(struct bch_geometry *geo,
+					   struct mtd_info *mtd)
 {
-	uint32_t ecc_strength;
-	ecc_strength = mxs_nand_get_ecc_strength(mtd->writesize, mtd->oobsize);
-	return mxs_nand_get_mark_offset(mtd->writesize, ecc_strength) & 0x7;
+	unsigned int max_ecc_strength_supported;
+
+	/* The default for the length of Galois Field. */
+	geo->gf_len = 13;
+
+	/* The default for chunk size. */
+	geo->ecc_chunk_size = 512;
+
+	if (geo->ecc_chunk_size < mtd->oobsize) {
+		geo->gf_len = 14;
+		geo->ecc_chunk_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;
+
+	/* Refer to Chapter 17 for i.MX6DQ, Chapter 18 for i.MX6SX */
+	if (is_mx6sx() || is_mx7())
+		max_ecc_strength_supported = 62;
+	else
+		max_ecc_strength_supported = 40;
+
+	/*
+	 * Determine the ECC layout with the formula:
+	 *	ECC bits per chunk = (total page spare data bits) /
+	 *		(bits per ECC level) / (chunks per page)
+	 * where:
+	 *	total page spare data bits =
+	 *		(page oob size - meta data size) * (bits per byte)
+	 */
+	geo->ecc_strength = ((mtd->oobsize - MXS_NAND_METADATA_SIZE) * 8)
+			/ (geo->gf_len * geo->ecc_chunk_count);
+
+	geo->ecc_strength = min(round_down(geo->ecc_strength, 2), max_ecc_strength_supported);
+
+	if (mxs_nand_calc_mark_offset(geo, mtd->writesize) < 0)
+		return -EINVAL;
+
+	return 0;
 }
 
 /*
@@ -380,18 +398,15 @@ static void mxs_nand_select_chip(struct mtd_info *mtd, int chip)
  * swapping the block mark, or swapping it *back* -- but it doesn't matter
  * because the the operation is the same.
  */
-static void mxs_nand_swap_block_mark(struct mtd_info *mtd,
-					uint8_t *data_buf, uint8_t *oob_buf)
+static void mxs_nand_swap_block_mark(struct bch_geometry *geo,
+				     uint8_t *data_buf, uint8_t *oob_buf)
 {
-	uint32_t bit_offset;
-	uint32_t buf_offset;
+	uint32_t bit_offset = geo->block_mark_bit_offset;
+	uint32_t buf_offset = geo->block_mark_byte_offset;
 
 	uint32_t src;
 	uint32_t dst;
 
-	bit_offset = mxs_nand_mark_bit_offset(mtd);
-	buf_offset = mxs_nand_mark_byte_offset(mtd);
-
 	/*
 	 * Get the byte from the data area that overlays the block mark. Since
 	 * the ECC engine applies its own view to the bits in the page, the
@@ -567,6 +582,7 @@ static int mxs_nand_ecc_read_page(struct mtd_info *mtd, struct nand_chip *nand,
 					int page)
 {
 	struct mxs_nand_info *nand_info = nand_get_controller_data(nand);
+	struct bch_geometry *geo = &nand_info->bch_geometry;
 	struct mxs_dma_desc *d;
 	uint32_t channel = MXS_DMA_CHANNEL_AHB_APBH_GPMI0 + nand_info->cur_chip;
 	uint32_t corrected = 0, failed = 0;
@@ -665,11 +681,11 @@ static int mxs_nand_ecc_read_page(struct mtd_info *mtd, struct nand_chip *nand,
 	mxs_nand_inval_data_buf(nand_info);
 
 	/* Read DMA completed, now do the mark swapping. */
-	mxs_nand_swap_block_mark(mtd, nand_info->data_buf, nand_info->oob_buf);
+	mxs_nand_swap_block_mark(geo, nand_info->data_buf, nand_info->oob_buf);
 
 	/* Loop over status bytes, accumulating ECC status. */
 	status = nand_info->oob_buf + mxs_nand_aux_status_offset();
-	for (i = 0; i < mxs_nand_ecc_chunk_cnt(mtd->writesize); i++) {
+	for (i = 0; i < geo->ecc_chunk_count; i++) {
 		if (status[i] == 0x00)
 			continue;
 
@@ -717,6 +733,7 @@ static int mxs_nand_ecc_write_page(struct mtd_info *mtd,
 				int oob_required, int page)
 {
 	struct mxs_nand_info *nand_info = nand_get_controller_data(nand);
+	struct bch_geometry *geo = &nand_info->bch_geometry;
 	struct mxs_dma_desc *d;
 	uint32_t channel = MXS_DMA_CHANNEL_AHB_APBH_GPMI0 + nand_info->cur_chip;
 	int ret;
@@ -725,7 +742,7 @@ static int mxs_nand_ecc_write_page(struct mtd_info *mtd,
 	memcpy(nand_info->oob_buf, nand->oob_poi, mtd->oobsize);
 
 	/* Handle block mark swapping. */
-	mxs_nand_swap_block_mark(mtd, nand_info->data_buf, nand_info->oob_buf);
+	mxs_nand_swap_block_mark(geo, nand_info->data_buf, nand_info->oob_buf);
 
 	/* Compile the DMA descriptor - write data. */
 	d = mxs_nand_get_dma_desc(nand_info);
@@ -986,39 +1003,30 @@ static int mxs_nand_setup_ecc(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;
 	struct mxs_bch_regs *bch_regs = (struct mxs_bch_regs *)MXS_BCH_BASE;
 	uint32_t tmp;
 
-	if (mtd->oobsize > MXS_NAND_CHUNK_DATA_CHUNK_SIZE) {
-		galois_field = 14;
-		chunk_data_size = MXS_NAND_CHUNK_DATA_CHUNK_SIZE * 2;
-	}
-
-	if (mtd->oobsize > chunk_data_size) {
-		printf("Not support the NAND chips whose oob size is larger then %d bytes!\n", chunk_data_size);
+	if (mxs_nand_calc_ecc_layout(geo, mtd))
 		return -EINVAL;
-	}
 
 	/* Configure BCH and set NFC geometry */
 	mxs_reset_block(&bch_regs->hw_bch_ctrl_reg);
 
 	/* Configure layout 0 */
-	tmp = (mxs_nand_ecc_chunk_cnt(mtd->writesize) - 1)
-		<< BCH_FLASHLAYOUT0_NBLOCKS_OFFSET;
+	tmp = (geo->ecc_chunk_count - 1) << BCH_FLASHLAYOUT0_NBLOCKS_OFFSET;
 	tmp |= MXS_NAND_METADATA_SIZE << BCH_FLASHLAYOUT0_META_SIZE_OFFSET;
-	tmp |= (mxs_nand_get_ecc_strength(mtd->writesize, mtd->oobsize) >> 1)
-		<< BCH_FLASHLAYOUT0_ECC0_OFFSET;
-	tmp |= chunk_data_size >> MXS_NAND_CHUNK_DATA_CHUNK_SIZE_SHIFT;
-	tmp |= (14 == galois_field ? 1 : 0) <<
+	tmp |= (geo->ecc_strength >> 1) << BCH_FLASHLAYOUT0_ECC0_OFFSET;
+	tmp |= geo->ecc_chunk_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);
 
 	tmp = (mtd->writesize + mtd->oobsize)
 		<< BCH_FLASHLAYOUT1_PAGE_SIZE_OFFSET;
-	tmp |= (mxs_nand_get_ecc_strength(mtd->writesize, mtd->oobsize) >> 1)
-		<< BCH_FLASHLAYOUT1_ECCN_OFFSET;
-	tmp |= chunk_data_size >> MXS_NAND_CHUNK_DATA_CHUNK_SIZE_SHIFT;
-	tmp |= (14 == galois_field ? 1 : 0) <<
+	tmp |= (geo->ecc_strength >> 1) << BCH_FLASHLAYOUT1_ECCN_OFFSET;
+	tmp |= geo->ecc_chunk_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);
 
-- 
2.17.0

[U-Boot] [PATCH v3 4/5] mtd: nand: mxs_nand: report correct ECC parameters

[Stefan Agner]

From: Stefan Agner <stefan.agner@toradex.com>

Report correct ECC parameters back to the stack. Do not report
bytes as we have it not immeaditly available and the Linux version
also does not report it. It seems to have no aversive effect.

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
---

Changes in v3: None
Changes in v2:
- Extend the patchset with "report correct ECC parameters" patch

 drivers/mtd/nand/mxs_nand.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c
index aa4c5a6526..2696b543ef 100644
--- a/drivers/mtd/nand/mxs_nand.c
+++ b/drivers/mtd/nand/mxs_nand.c
@@ -1212,9 +1212,8 @@ void board_nand_init(void)
 
 	nand->ecc.layout	= &fake_ecc_layout;
 	nand->ecc.mode		= NAND_ECC_HW;
-	nand->ecc.bytes		= 9;
-	nand->ecc.size		= 512;
-	nand->ecc.strength	= 8;
+	nand->ecc.size		= nand_info->bch_geometry.ecc_chunk_size;
+	nand->ecc.strength	= nand_info->bch_geometry.ecc_strength;
 
 	/* second phase scan */
 	err = nand_scan_tail(mtd);
-- 
2.17.0

[U-Boot] [PATCH v3 5/5] mtd: nand: mxs_nand: add minimal ECC support

[Stefan Agner]

From: Stefan Agner <stefan.agner@toradex.com>

Add support for minimum ECC strength supported by the NAND chip.
This aligns with the behavior when using the fsl,use-minimum-ecc
device tree property in Linux.

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
---

Changes in v3: None
Changes in v2: None

 drivers/mtd/nand/Kconfig    |  8 +++++
 drivers/mtd/nand/mxs_nand.c | 71 +++++++++++++++++++++++++++++--------
 2 files changed, 65 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 4db259fcb2..c039b9cc60 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -152,6 +152,14 @@ config NAND_MXS
 	  This enables NAND driver for the NAND flash controller on the
 	  MXS processors.
 
+if NAND_MXS
+
+config NAND_MXS_USE_MINIMUM_ECC
+	bool "Use minimum ECC strength supported by the controller"
+	default false
+
+endif
+
 config NAND_ZYNQ
 	bool "Support for Zynq Nand controller"
 	select SYS_NAND_SELF_INIT
diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c
index 2696b543ef..8305bf2302 100644
--- a/drivers/mtd/nand/mxs_nand.c
+++ b/drivers/mtd/nand/mxs_nand.c
@@ -211,11 +211,52 @@ static inline int mxs_nand_calc_mark_offset(struct bch_geometry *geo,
 	return 0;
 }
 
+static inline unsigned int mxs_nand_max_ecc_strength_supported(void)
+{
+	/* Refer to Chapter 17 for i.MX6DQ, Chapter 18 for i.MX6SX */
+	if (is_mx6sx() || is_mx7())
+		return 62;
+	else
+		return 40;
+}
+
+static inline int mxs_nand_calc_ecc_layout_by_info(struct bch_geometry *geo,
+						   struct mtd_info *mtd)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+
+	if (!(chip->ecc_strength_ds > 0 && chip->ecc_step_ds > 0))
+		return -ENOTSUPP;
+
+	switch (chip->ecc_step_ds) {
+	case SZ_512:
+		geo->gf_len = 13;
+		break;
+	case SZ_1K:
+		geo->gf_len = 14;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	geo->ecc_chunk_size = chip->ecc_step_ds;
+	geo->ecc_strength = round_up(chip->ecc_strength_ds, 2);
+
+	/* Keep the C >= O */
+	if (geo->ecc_chunk_size < mtd->oobsize)
+		return -EINVAL;
+
+	if (geo->ecc_strength > mxs_nand_max_ecc_strength_supported())
+		return -EINVAL;
+
+	geo->ecc_chunk_count = mtd->writesize / geo->ecc_chunk_size;
+
+	return 0;
+}
+
 static inline int mxs_nand_calc_ecc_layout(struct bch_geometry *geo,
 					   struct mtd_info *mtd)
 {
-	unsigned int max_ecc_strength_supported;
-
 	/* The default for the length of Galois Field. */
 	geo->gf_len = 13;
 
@@ -235,12 +276,6 @@ static inline int mxs_nand_calc_ecc_layout(struct bch_geometry *geo,
 
 	geo->ecc_chunk_count = mtd->writesize / geo->ecc_chunk_size;
 
-	/* Refer to Chapter 17 for i.MX6DQ, Chapter 18 for i.MX6SX */
-	if (is_mx6sx() || is_mx7())
-		max_ecc_strength_supported = 62;
-	else
-		max_ecc_strength_supported = 40;
-
 	/*
 	 * Determine the ECC layout with the formula:
 	 *	ECC bits per chunk = (total page spare data bits) /
@@ -252,10 +287,8 @@ static inline int mxs_nand_calc_ecc_layout(struct bch_geometry *geo,
 	geo->ecc_strength = ((mtd->oobsize - MXS_NAND_METADATA_SIZE) * 8)
 			/ (geo->gf_len * geo->ecc_chunk_count);
 
-	geo->ecc_strength = min(round_down(geo->ecc_strength, 2), max_ecc_strength_supported);
-
-	if (mxs_nand_calc_mark_offset(geo, mtd->writesize) < 0)
-		return -EINVAL;
+	geo->ecc_strength = min(round_down(geo->ecc_strength, 2),
+				mxs_nand_max_ecc_strength_supported());
 
 	return 0;
 }
@@ -1006,9 +1039,19 @@ static int mxs_nand_setup_ecc(struct mtd_info *mtd)
 	struct bch_geometry *geo = &nand_info->bch_geometry;
 	struct mxs_bch_regs *bch_regs = (struct mxs_bch_regs *)MXS_BCH_BASE;
 	uint32_t tmp;
+	int ret = -ENOTSUPP;
 
-	if (mxs_nand_calc_ecc_layout(geo, mtd))
-		return -EINVAL;
+#ifdef CONFIG_NAND_MXS_USE_MINIMUM_ECC
+	ret = mxs_nand_calc_ecc_layout_by_info(geo, mtd);
+#endif
+
+	if (ret == -ENOTSUPP)
+		ret = mxs_nand_calc_ecc_layout(geo, 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);
-- 
2.17.0

[U-Boot] [PATCH v3 5/5] mtd: nand: mxs_nand: add minimal ECC support

[Stefano Babic]

Hi Stefan,

On 11/04/2018 18:04, Stefan Agner wrote:
> From: Stefan Agner <stefan.agner@toradex.com>
> 
> Add support for minimum ECC strength supported by the NAND chip.
> This aligns with the behavior when using the fsl,use-minimum-ecc
> device tree property in Linux.
> 
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/mtd/nand/Kconfig    |  8 +++++
>  drivers/mtd/nand/mxs_nand.c | 71 +++++++++++++++++++++++++++++--------
>  2 files changed, 65 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index 4db259fcb2..c039b9cc60 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -152,6 +152,14 @@ config NAND_MXS
>  	  This enables NAND driver for the NAND flash controller on the
>  	  MXS processors.
>  
> +if NAND_MXS
> +
> +config NAND_MXS_USE_MINIMUM_ECC
> +	bool "Use minimum ECC strength supported by the controller"
> +	default false
> +
> +endif
> +
>  config NAND_ZYNQ
>  	bool "Support for Zynq Nand controller"
>  	select SYS_NAND_SELF_INIT
> diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c
> index 2696b543ef..8305bf2302 100644
> --- a/drivers/mtd/nand/mxs_nand.c
> +++ b/drivers/mtd/nand/mxs_nand.c
> @@ -211,11 +211,52 @@ static inline int mxs_nand_calc_mark_offset(struct bch_geometry *geo,
>  	return 0;
>  }
>  
> +static inline unsigned int mxs_nand_max_ecc_strength_supported(void)
> +{
> +	/* Refer to Chapter 17 for i.MX6DQ, Chapter 18 for i.MX6SX */
> +	if (is_mx6sx() || is_mx7())
> +		return 62;
> +	else
> +		return 40;
> +}
> +
> +static inline int mxs_nand_calc_ecc_layout_by_info(struct bch_geometry *geo,
> +						   struct mtd_info *mtd)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +
> +	if (!(chip->ecc_strength_ds > 0 && chip->ecc_step_ds > 0))
> +		return -ENOTSUPP;
> +
> +	switch (chip->ecc_step_ds) {
> +	case SZ_512:
> +		geo->gf_len = 13;
> +		break;
> +	case SZ_1K:
> +		geo->gf_len = 14;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	geo->ecc_chunk_size = chip->ecc_step_ds;
> +	geo->ecc_strength = round_up(chip->ecc_strength_ds, 2);
> +
> +	/* Keep the C >= O */
> +	if (geo->ecc_chunk_size < mtd->oobsize)
> +		return -EINVAL;
> +
> +	if (geo->ecc_strength > mxs_nand_max_ecc_strength_supported())
> +		return -EINVAL;
> +
> +	geo->ecc_chunk_count = mtd->writesize / geo->ecc_chunk_size;
> +
> +	return 0;
> +}
> +
>  static inline int mxs_nand_calc_ecc_layout(struct bch_geometry *geo,
>  					   struct mtd_info *mtd)
>  {
> -	unsigned int max_ecc_strength_supported;
> -
>  	/* The default for the length of Galois Field. */
>  	geo->gf_len = 13;
>  
> @@ -235,12 +276,6 @@ static inline int mxs_nand_calc_ecc_layout(struct bch_geometry *geo,
>  
>  	geo->ecc_chunk_count = mtd->writesize / geo->ecc_chunk_size;
>  
> -	/* Refer to Chapter 17 for i.MX6DQ, Chapter 18 for i.MX6SX */
> -	if (is_mx6sx() || is_mx7())
> -		max_ecc_strength_supported = 62;
> -	else
> -		max_ecc_strength_supported = 40;
> -
>  	/*
>  	 * Determine the ECC layout with the formula:
>  	 *	ECC bits per chunk = (total page spare data bits) /
> @@ -252,10 +287,8 @@ static inline int mxs_nand_calc_ecc_layout(struct bch_geometry *geo,
>  	geo->ecc_strength = ((mtd->oobsize - MXS_NAND_METADATA_SIZE) * 8)
>  			/ (geo->gf_len * geo->ecc_chunk_count);
>  
> -	geo->ecc_strength = min(round_down(geo->ecc_strength, 2), max_ecc_strength_supported);
> -
> -	if (mxs_nand_calc_mark_offset(geo, mtd->writesize) < 0)
> -		return -EINVAL;
> +	geo->ecc_strength = min(round_down(geo->ecc_strength, 2),
> +				mxs_nand_max_ecc_strength_supported());
>  
>  	return 0;
>  }
> @@ -1006,9 +1039,19 @@ static int mxs_nand_setup_ecc(struct mtd_info *mtd)
>  	struct bch_geometry *geo = &nand_info->bch_geometry;
>  	struct mxs_bch_regs *bch_regs = (struct mxs_bch_regs *)MXS_BCH_BASE;
>  	uint32_t tmp;
> +	int ret = -ENOTSUPP;
>  
> -	if (mxs_nand_calc_ecc_layout(geo, mtd))
> -		return -EINVAL;
> +#ifdef CONFIG_NAND_MXS_USE_MINIMUM_ECC
> +	ret = mxs_nand_calc_ecc_layout_by_info(geo, mtd);
> +#endif
> +
> +	if (ret == -ENOTSUPP)
> +		ret = mxs_nand_calc_ecc_layout(geo, 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);
> 

This is not completely clear to me - is this series replaced by :

	http://patchwork.ozlabs.org/patch/902010/

Regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH v3 5/5] mtd: nand: mxs_nand: add minimal ECC support

[Stefan Agner]

On 27.04.2018 09:31, Stefano Babic wrote:
> Hi Stefan,
> 
> On 11/04/2018 18:04, Stefan Agner wrote:
>> From: Stefan Agner <stefan.agner@toradex.com>
>>
>> Add support for minimum ECC strength supported by the NAND chip.
>> This aligns with the behavior when using the fsl,use-minimum-ecc
>> device tree property in Linux.
>>
>> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
>> ---
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>>  drivers/mtd/nand/Kconfig    |  8 +++++
>>  drivers/mtd/nand/mxs_nand.c | 71 +++++++++++++++++++++++++++++--------
>>  2 files changed, 65 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
>> index 4db259fcb2..c039b9cc60 100644
>> --- a/drivers/mtd/nand/Kconfig
>> +++ b/drivers/mtd/nand/Kconfig
>> @@ -152,6 +152,14 @@ config NAND_MXS
>>  	  This enables NAND driver for the NAND flash controller on the
>>  	  MXS processors.
>>
>> +if NAND_MXS
>> +
>> +config NAND_MXS_USE_MINIMUM_ECC
>> +	bool "Use minimum ECC strength supported by the controller"
>> +	default false
>> +
>> +endif
>> +
>>  config NAND_ZYNQ
>>  	bool "Support for Zynq Nand controller"
>>  	select SYS_NAND_SELF_INIT
>> diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c
>> index 2696b543ef..8305bf2302 100644
>> --- a/drivers/mtd/nand/mxs_nand.c
>> +++ b/drivers/mtd/nand/mxs_nand.c
>> @@ -211,11 +211,52 @@ static inline int mxs_nand_calc_mark_offset(struct bch_geometry *geo,
>>  	return 0;
>>  }
>>
>> +static inline unsigned int mxs_nand_max_ecc_strength_supported(void)
>> +{
>> +	/* Refer to Chapter 17 for i.MX6DQ, Chapter 18 for i.MX6SX */
>> +	if (is_mx6sx() || is_mx7())
>> +		return 62;
>> +	else
>> +		return 40;
>> +}
>> +
>> +static inline int mxs_nand_calc_ecc_layout_by_info(struct bch_geometry *geo,
>> +						   struct mtd_info *mtd)
>> +{
>> +	struct nand_chip *chip = mtd_to_nand(mtd);
>> +
>> +	if (!(chip->ecc_strength_ds > 0 && chip->ecc_step_ds > 0))
>> +		return -ENOTSUPP;
>> +
>> +	switch (chip->ecc_step_ds) {
>> +	case SZ_512:
>> +		geo->gf_len = 13;
>> +		break;
>> +	case SZ_1K:
>> +		geo->gf_len = 14;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	geo->ecc_chunk_size = chip->ecc_step_ds;
>> +	geo->ecc_strength = round_up(chip->ecc_strength_ds, 2);
>> +
>> +	/* Keep the C >= O */
>> +	if (geo->ecc_chunk_size < mtd->oobsize)
>> +		return -EINVAL;
>> +
>> +	if (geo->ecc_strength > mxs_nand_max_ecc_strength_supported())
>> +		return -EINVAL;
>> +
>> +	geo->ecc_chunk_count = mtd->writesize / geo->ecc_chunk_size;
>> +
>> +	return 0;
>> +}
>> +
>>  static inline int mxs_nand_calc_ecc_layout(struct bch_geometry *geo,
>>  					   struct mtd_info *mtd)
>>  {
>> -	unsigned int max_ecc_strength_supported;
>> -
>>  	/* The default for the length of Galois Field. */
>>  	geo->gf_len = 13;
>>
>> @@ -235,12 +276,6 @@ static inline int mxs_nand_calc_ecc_layout(struct bch_geometry *geo,
>>
>>  	geo->ecc_chunk_count = mtd->writesize / geo->ecc_chunk_size;
>>
>> -	/* Refer to Chapter 17 for i.MX6DQ, Chapter 18 for i.MX6SX */
>> -	if (is_mx6sx() || is_mx7())
>> -		max_ecc_strength_supported = 62;
>> -	else
>> -		max_ecc_strength_supported = 40;
>> -
>>  	/*
>>  	 * Determine the ECC layout with the formula:
>>  	 *	ECC bits per chunk = (total page spare data bits) /
>> @@ -252,10 +287,8 @@ static inline int mxs_nand_calc_ecc_layout(struct bch_geometry *geo,
>>  	geo->ecc_strength = ((mtd->oobsize - MXS_NAND_METADATA_SIZE) * 8)
>>  			/ (geo->gf_len * geo->ecc_chunk_count);
>>
>> -	geo->ecc_strength = min(round_down(geo->ecc_strength, 2), max_ecc_strength_supported);
>> -
>> -	if (mxs_nand_calc_mark_offset(geo, mtd->writesize) < 0)
>> -		return -EINVAL;
>> +	geo->ecc_strength = min(round_down(geo->ecc_strength, 2),
>> +				mxs_nand_max_ecc_strength_supported());
>>
>>  	return 0;
>>  }
>> @@ -1006,9 +1039,19 @@ static int mxs_nand_setup_ecc(struct mtd_info *mtd)
>>  	struct bch_geometry *geo = &nand_info->bch_geometry;
>>  	struct mxs_bch_regs *bch_regs = (struct mxs_bch_regs *)MXS_BCH_BASE;
>>  	uint32_t tmp;
>> +	int ret = -ENOTSUPP;
>>
>> -	if (mxs_nand_calc_ecc_layout(geo, mtd))
>> -		return -EINVAL;
>> +#ifdef CONFIG_NAND_MXS_USE_MINIMUM_ECC
>> +	ret = mxs_nand_calc_ecc_layout_by_info(geo, mtd);
>> +#endif
>> +
>> +	if (ret == -ENOTSUPP)
>> +		ret = mxs_nand_calc_ecc_layout(geo, 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);
>>
> 
> This is not completely clear to me - is this series replaced by :
> 
> 	http://patchwork.ozlabs.org/patch/902010/

No, the other patch set is on-top of this work.

With this patch set I already introduce minimum ECC, but by just using a
config symbol. The other patchset allows to enable the same using the
device tree property.

I guess the config symbol is still useful for non-dt platforms or SPLs.

--
Stefan

[U-Boot] [PATCH v3 5/5] mtd: nand: mxs_nand: add minimal ECC support

[Stefan Agner]

Hi Stefano,

On 30.04.2018 10:08, Stefan Agner wrote:
> On 27.04.2018 09:31, Stefano Babic wrote:
>> Hi Stefan,
>>
>> On 11/04/2018 18:04, Stefan Agner wrote:
>>> From: Stefan Agner <stefan.agner@toradex.com>
>>>
>>> Add support for minimum ECC strength supported by the NAND chip.
>>> This aligns with the behavior when using the fsl,use-minimum-ecc
>>> device tree property in Linux.
>>>
>>> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
>>> ---
>>>
>>> Changes in v3: None
>>> Changes in v2: None
>>>
>>>  drivers/mtd/nand/Kconfig    |  8 +++++
>>>  drivers/mtd/nand/mxs_nand.c | 71 +++++++++++++++++++++++++++++--------
>>>  2 files changed, 65 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
>>> index 4db259fcb2..c039b9cc60 100644
>>> --- a/drivers/mtd/nand/Kconfig
>>> +++ b/drivers/mtd/nand/Kconfig
>>> @@ -152,6 +152,14 @@ config NAND_MXS
>>>  	  This enables NAND driver for the NAND flash controller on the
>>>  	  MXS processors.
>>>
>>> +if NAND_MXS
>>> +
>>> +config NAND_MXS_USE_MINIMUM_ECC
>>> +	bool "Use minimum ECC strength supported by the controller"
>>> +	default false
>>> +
>>> +endif
>>> +
>>>  config NAND_ZYNQ
>>>  	bool "Support for Zynq Nand controller"
>>>  	select SYS_NAND_SELF_INIT
>>> diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c
>>> index 2696b543ef..8305bf2302 100644
>>> --- a/drivers/mtd/nand/mxs_nand.c
>>> +++ b/drivers/mtd/nand/mxs_nand.c
>>> @@ -211,11 +211,52 @@ static inline int mxs_nand_calc_mark_offset(struct bch_geometry *geo,
>>>  	return 0;
>>>  }
>>>
>>> +static inline unsigned int mxs_nand_max_ecc_strength_supported(void)
>>> +{
>>> +	/* Refer to Chapter 17 for i.MX6DQ, Chapter 18 for i.MX6SX */
>>> +	if (is_mx6sx() || is_mx7())
>>> +		return 62;
>>> +	else
>>> +		return 40;
>>> +}
>>> +
>>> +static inline int mxs_nand_calc_ecc_layout_by_info(struct bch_geometry *geo,
>>> +						   struct mtd_info *mtd)
>>> +{
>>> +	struct nand_chip *chip = mtd_to_nand(mtd);
>>> +
>>> +	if (!(chip->ecc_strength_ds > 0 && chip->ecc_step_ds > 0))
>>> +		return -ENOTSUPP;
>>> +
>>> +	switch (chip->ecc_step_ds) {
>>> +	case SZ_512:
>>> +		geo->gf_len = 13;
>>> +		break;
>>> +	case SZ_1K:
>>> +		geo->gf_len = 14;
>>> +		break;
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	geo->ecc_chunk_size = chip->ecc_step_ds;
>>> +	geo->ecc_strength = round_up(chip->ecc_strength_ds, 2);
>>> +
>>> +	/* Keep the C >= O */
>>> +	if (geo->ecc_chunk_size < mtd->oobsize)
>>> +		return -EINVAL;
>>> +
>>> +	if (geo->ecc_strength > mxs_nand_max_ecc_strength_supported())
>>> +		return -EINVAL;
>>> +
>>> +	geo->ecc_chunk_count = mtd->writesize / geo->ecc_chunk_size;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  static inline int mxs_nand_calc_ecc_layout(struct bch_geometry *geo,
>>>  					   struct mtd_info *mtd)
>>>  {
>>> -	unsigned int max_ecc_strength_supported;
>>> -
>>>  	/* The default for the length of Galois Field. */
>>>  	geo->gf_len = 13;
>>>
>>> @@ -235,12 +276,6 @@ static inline int mxs_nand_calc_ecc_layout(struct bch_geometry *geo,
>>>
>>>  	geo->ecc_chunk_count = mtd->writesize / geo->ecc_chunk_size;
>>>
>>> -	/* Refer to Chapter 17 for i.MX6DQ, Chapter 18 for i.MX6SX */
>>> -	if (is_mx6sx() || is_mx7())
>>> -		max_ecc_strength_supported = 62;
>>> -	else
>>> -		max_ecc_strength_supported = 40;
>>> -
>>>  	/*
>>>  	 * Determine the ECC layout with the formula:
>>>  	 *	ECC bits per chunk = (total page spare data bits) /
>>> @@ -252,10 +287,8 @@ static inline int mxs_nand_calc_ecc_layout(struct bch_geometry *geo,
>>>  	geo->ecc_strength = ((mtd->oobsize - MXS_NAND_METADATA_SIZE) * 8)
>>>  			/ (geo->gf_len * geo->ecc_chunk_count);
>>>
>>> -	geo->ecc_strength = min(round_down(geo->ecc_strength, 2), max_ecc_strength_supported);
>>> -
>>> -	if (mxs_nand_calc_mark_offset(geo, mtd->writesize) < 0)
>>> -		return -EINVAL;
>>> +	geo->ecc_strength = min(round_down(geo->ecc_strength, 2),
>>> +				mxs_nand_max_ecc_strength_supported());
>>>
>>>  	return 0;
>>>  }
>>> @@ -1006,9 +1039,19 @@ static int mxs_nand_setup_ecc(struct mtd_info *mtd)
>>>  	struct bch_geometry *geo = &nand_info->bch_geometry;
>>>  	struct mxs_bch_regs *bch_regs = (struct mxs_bch_regs *)MXS_BCH_BASE;
>>>  	uint32_t tmp;
>>> +	int ret = -ENOTSUPP;
>>>
>>> -	if (mxs_nand_calc_ecc_layout(geo, mtd))
>>> -		return -EINVAL;
>>> +#ifdef CONFIG_NAND_MXS_USE_MINIMUM_ECC
>>> +	ret = mxs_nand_calc_ecc_layout_by_info(geo, mtd);
>>> +#endif
>>> +
>>> +	if (ret == -ENOTSUPP)
>>> +		ret = mxs_nand_calc_ecc_layout(geo, 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);
>>>
>>
>> This is not completely clear to me - is this series replaced by :
>>
>> 	http://patchwork.ozlabs.org/patch/902010/
> 
> No, the other patch set is on-top of this work.
> 
> With this patch set I already introduce minimum ECC, but by just using a
> config symbol. The other patchset allows to enable the same using the
> device tree property.
> 
> I guess the config symbol is still useful for non-dt platforms or SPLs.

Is there anything stopping these two patchset from getting merged?

--
Stefan

[U-Boot] [PATCH v3 5/5] mtd: nand: mxs_nand: add minimal ECC support

[Stefan Agner]

Tom, Stefano,

Scott Wood seems not to be very active this days in the U-Boot
community.

This patchsets seem all to be blocked due to that:
https://patchwork.ozlabs.org/cover/897263/
https://patchwork.ozlabs.org/cover/901995/
https://patchwork.ozlabs.org/patch/922907/

Those patches have been around quite a while and I tested them well. Any
change to get that still in?

Best regards,
Stefan


On 30.05.2018 16:51, Stefan Agner wrote:
> Hi Stefano,
> 
> On 30.04.2018 10:08, Stefan Agner wrote:
>> On 27.04.2018 09:31, Stefano Babic wrote:
>>> Hi Stefan,
>>>
>>> On 11/04/2018 18:04, Stefan Agner wrote:
>>>> From: Stefan Agner <stefan.agner@toradex.com>
>>>>
>>>> Add support for minimum ECC strength supported by the NAND chip.
>>>> This aligns with the behavior when using the fsl,use-minimum-ecc
>>>> device tree property in Linux.
>>>>
>>>> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
>>>> ---
>>>>
>>>> Changes in v3: None
>>>> Changes in v2: None
>>>>
>>>>  drivers/mtd/nand/Kconfig    |  8 +++++
>>>>  drivers/mtd/nand/mxs_nand.c | 71 +++++++++++++++++++++++++++++--------
>>>>  2 files changed, 65 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
>>>> index 4db259fcb2..c039b9cc60 100644
>>>> --- a/drivers/mtd/nand/Kconfig
>>>> +++ b/drivers/mtd/nand/Kconfig
>>>> @@ -152,6 +152,14 @@ config NAND_MXS
>>>>  	  This enables NAND driver for the NAND flash controller on the
>>>>  	  MXS processors.
>>>>
>>>> +if NAND_MXS
>>>> +
>>>> +config NAND_MXS_USE_MINIMUM_ECC
>>>> +	bool "Use minimum ECC strength supported by the controller"
>>>> +	default false
>>>> +
>>>> +endif
>>>> +
>>>>  config NAND_ZYNQ
>>>>  	bool "Support for Zynq Nand controller"
>>>>  	select SYS_NAND_SELF_INIT
>>>> diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c
>>>> index 2696b543ef..8305bf2302 100644
>>>> --- a/drivers/mtd/nand/mxs_nand.c
>>>> +++ b/drivers/mtd/nand/mxs_nand.c
>>>> @@ -211,11 +211,52 @@ static inline int mxs_nand_calc_mark_offset(struct bch_geometry *geo,
>>>>  	return 0;
>>>>  }
>>>>
>>>> +static inline unsigned int mxs_nand_max_ecc_strength_supported(void)
>>>> +{
>>>> +	/* Refer to Chapter 17 for i.MX6DQ, Chapter 18 for i.MX6SX */
>>>> +	if (is_mx6sx() || is_mx7())
>>>> +		return 62;
>>>> +	else
>>>> +		return 40;
>>>> +}
>>>> +
>>>> +static inline int mxs_nand_calc_ecc_layout_by_info(struct bch_geometry *geo,
>>>> +						   struct mtd_info *mtd)
>>>> +{
>>>> +	struct nand_chip *chip = mtd_to_nand(mtd);
>>>> +
>>>> +	if (!(chip->ecc_strength_ds > 0 && chip->ecc_step_ds > 0))
>>>> +		return -ENOTSUPP;
>>>> +
>>>> +	switch (chip->ecc_step_ds) {
>>>> +	case SZ_512:
>>>> +		geo->gf_len = 13;
>>>> +		break;
>>>> +	case SZ_1K:
>>>> +		geo->gf_len = 14;
>>>> +		break;
>>>> +	default:
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	geo->ecc_chunk_size = chip->ecc_step_ds;
>>>> +	geo->ecc_strength = round_up(chip->ecc_strength_ds, 2);
>>>> +
>>>> +	/* Keep the C >= O */
>>>> +	if (geo->ecc_chunk_size < mtd->oobsize)
>>>> +		return -EINVAL;
>>>> +
>>>> +	if (geo->ecc_strength > mxs_nand_max_ecc_strength_supported())
>>>> +		return -EINVAL;
>>>> +
>>>> +	geo->ecc_chunk_count = mtd->writesize / geo->ecc_chunk_size;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>  static inline int mxs_nand_calc_ecc_layout(struct bch_geometry *geo,
>>>>  					   struct mtd_info *mtd)
>>>>  {
>>>> -	unsigned int max_ecc_strength_supported;
>>>> -
>>>>  	/* The default for the length of Galois Field. */
>>>>  	geo->gf_len = 13;
>>>>
>>>> @@ -235,12 +276,6 @@ static inline int mxs_nand_calc_ecc_layout(struct bch_geometry *geo,
>>>>
>>>>  	geo->ecc_chunk_count = mtd->writesize / geo->ecc_chunk_size;
>>>>
>>>> -	/* Refer to Chapter 17 for i.MX6DQ, Chapter 18 for i.MX6SX */
>>>> -	if (is_mx6sx() || is_mx7())
>>>> -		max_ecc_strength_supported = 62;
>>>> -	else
>>>> -		max_ecc_strength_supported = 40;
>>>> -
>>>>  	/*
>>>>  	 * Determine the ECC layout with the formula:
>>>>  	 *	ECC bits per chunk = (total page spare data bits) /
>>>> @@ -252,10 +287,8 @@ static inline int mxs_nand_calc_ecc_layout(struct bch_geometry *geo,
>>>>  	geo->ecc_strength = ((mtd->oobsize - MXS_NAND_METADATA_SIZE) * 8)
>>>>  			/ (geo->gf_len * geo->ecc_chunk_count);
>>>>
>>>> -	geo->ecc_strength = min(round_down(geo->ecc_strength, 2), max_ecc_strength_supported);
>>>> -
>>>> -	if (mxs_nand_calc_mark_offset(geo, mtd->writesize) < 0)
>>>> -		return -EINVAL;
>>>> +	geo->ecc_strength = min(round_down(geo->ecc_strength, 2),
>>>> +				mxs_nand_max_ecc_strength_supported());
>>>>
>>>>  	return 0;
>>>>  }
>>>> @@ -1006,9 +1039,19 @@ static int mxs_nand_setup_ecc(struct mtd_info *mtd)
>>>>  	struct bch_geometry *geo = &nand_info->bch_geometry;
>>>>  	struct mxs_bch_regs *bch_regs = (struct mxs_bch_regs *)MXS_BCH_BASE;
>>>>  	uint32_t tmp;
>>>> +	int ret = -ENOTSUPP;
>>>>
>>>> -	if (mxs_nand_calc_ecc_layout(geo, mtd))
>>>> -		return -EINVAL;
>>>> +#ifdef CONFIG_NAND_MXS_USE_MINIMUM_ECC
>>>> +	ret = mxs_nand_calc_ecc_layout_by_info(geo, mtd);
>>>> +#endif
>>>> +
>>>> +	if (ret == -ENOTSUPP)
>>>> +		ret = mxs_nand_calc_ecc_layout(geo, 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);
>>>>
>>>
>>> This is not completely clear to me - is this series replaced by :
>>>
>>> 	http://patchwork.ozlabs.org/patch/902010/
>>
>> No, the other patch set is on-top of this work.
>>
>> With this patch set I already introduce minimum ECC, but by just using a
>> config symbol. The other patchset allows to enable the same using the
>> device tree property.
>>
>> I guess the config symbol is still useful for non-dt platforms or SPLs.
> 
> Is there anything stopping these two patchset from getting merged?
> 
> --
> Stefan
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH v3 5/5] mtd: nand: mxs_nand: add minimal ECC support

[Tom Rini]

On Thu, Jun 21, 2018 at 01:46:10PM +0200, Stefan Agner wrote:

> Tom, Stefano,
> 
> Scott Wood seems not to be very active this days in the U-Boot
> community.
> 
> This patchsets seem all to be blocked due to that:
> https://patchwork.ozlabs.org/cover/897263/
> https://patchwork.ozlabs.org/cover/901995/
> https://patchwork.ozlabs.org/patch/922907/
> 
> Those patches have been around quite a while and I tested them well. Any
> change to get that still in?

I don't object to those coming in via one of the SoC trees, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180621/ad3badb7/attachment.sig>

[U-Boot] [PATCH v3 5/5] mtd: nand: mxs_nand: add minimal ECC support

[Stefano Babic]

Hi Stefan,

On 21/06/2018 17:22, Tom Rini wrote:
> On Thu, Jun 21, 2018 at 01:46:10PM +0200, Stefan Agner wrote:
> 
>> Tom, Stefano,
>>
>> Scott Wood seems not to be very active this days in the U-Boot
>> community.
>>
>> This patchsets seem all to be blocked due to that:
>> https://patchwork.ozlabs.org/cover/897263/
>> https://patchwork.ozlabs.org/cover/901995/
>> https://patchwork.ozlabs.org/patch/922907/
>>
>> Those patches have been around quite a while and I tested them well. Any
>> change to get that still in?
> 
> I don't object to those coming in via one of the SoC trees, thanks!
> 

Ok - we have a general agreement, I pick your patches into u-boot-imx.

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH v3 5/5] mtd: nand: mxs_nand: add minimal ECC support

[Stefano Babic]

Hi Stefan,

On 21/06/2018 18:39, Stefano Babic wrote:
> Hi Stefan,
> 
> On 21/06/2018 17:22, Tom Rini wrote:
>> On Thu, Jun 21, 2018 at 01:46:10PM +0200, Stefan Agner wrote:
>>
>>> Tom, Stefano,
>>>
>>> Scott Wood seems not to be very active this days in the U-Boot
>>> community.
>>>
>>> This patchsets seem all to be blocked due to that:
>>> https://patchwork.ozlabs.org/cover/897263/
>>> https://patchwork.ozlabs.org/cover/901995/
>>> https://patchwork.ozlabs.org/patch/922907/
>>>
>>> Those patches have been around quite a while and I tested them well. Any
>>> change to get that still in?
>>
>> I don't object to those coming in via one of the SoC trees, thanks!
>>
> 
> Ok - we have a general agreement, I pick your patches into u-boot-imx.
> 

I get several broken boards and many warnings when I build with your
patches. Maybe have I missed some of them ? I have pushed a temporary
branch (nand-imx) on the server, let me know if I miss something.

There are 15 boards broken, errors seem trivial but I do not want to fix
anything without showing to you. There are mainly two issues, for example:

drivers/mtd/nand/mxs_nand_spl.c: In function ?mxs_nand_init?:
drivers/mtd/nand/mxs_nand_spl.c:148:2: error: too many arguments to
function ?board_nand_init?
  board_nand_init(&nand_chip);
  ^~~~~~~~~~~~~~~
In file included from drivers/mtd/nand/mxs_nand_spl.c:7:0:
include/nand.h:39:6: note: declared here
 void board_nand_init(void);
      ^~~~~~~~~~~~~~~
make[4]: *** [spl/drivers/mtd/nand/mxs_nand_spl.o] Error 1
make[3]: *** [spl/drivers/mtd/nand] Error 2
make[2]: *** [spl/drivers] Error 2
make[1]: *** [spl/u-boot-spl] Error 2
make: *** [sub-make] Error 2


The second one is a missing include, like in mx28evk:


drivers/mtd/nand/mxs_nand.c: In function ?mxs_nand_calc_ecc_layout_by_info?:
drivers/mtd/nand/mxs_nand.c:172:7: error: ?SZ_512? undeclared (first use
in this function)
  case SZ_512:
       ^~~~~~
drivers/mtd/nand/mxs_nand.c:172:7: note: each undeclared identifier is
reported only once for each function it appears in
drivers/mtd/nand/mxs_nand.c:175:7: error: ?SZ_1K? undeclared (first use
in this function)
  case SZ_1K:
       ^~~~~
make[2]: *** [drivers/mtd/nand/mxs_nand.o] Error 1
make[1]: *** [drivers/mtd/nand] Error 2
make: *** [sub-make] Error 2

Can you take a look ?

Thanks,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH v3 5/5] mtd: nand: mxs_nand: add minimal ECC support

[Stefan Agner]

On 22.06.2018 10:46, Stefano Babic wrote:
> Hi Stefan,
> 
> On 21/06/2018 18:39, Stefano Babic wrote:
>> Hi Stefan,
>>
>> On 21/06/2018 17:22, Tom Rini wrote:
>>> On Thu, Jun 21, 2018 at 01:46:10PM +0200, Stefan Agner wrote:
>>>
>>>> Tom, Stefano,
>>>>
>>>> Scott Wood seems not to be very active this days in the U-Boot
>>>> community.
>>>>
>>>> This patchsets seem all to be blocked due to that:
>>>> https://patchwork.ozlabs.org/cover/897263/
>>>> https://patchwork.ozlabs.org/cover/901995/
>>>> https://patchwork.ozlabs.org/patch/922907/
>>>>
>>>> Those patches have been around quite a while and I tested them well. Any
>>>> change to get that still in?
>>>
>>> I don't object to those coming in via one of the SoC trees, thanks!
>>>
>>
>> Ok - we have a general agreement, I pick your patches into u-boot-imx.
>>
> 
> I get several broken boards and many warnings when I build with your
> patches. Maybe have I missed some of them ? I have pushed a temporary
> branch (nand-imx) on the server, let me know if I miss something.

Patches look good.

> 
> There are 15 boards broken, errors seem trivial but I do not want to fix
> anything without showing to you. There are mainly two issues, for example:
> 
> drivers/mtd/nand/mxs_nand_spl.c: In function ?mxs_nand_init?:
> drivers/mtd/nand/mxs_nand_spl.c:148:2: error: too many arguments to
> function ?board_nand_init?
>   board_nand_init(&nand_chip);
>   ^~~~~~~~~~~~~~~
> In file included from drivers/mtd/nand/mxs_nand_spl.c:7:0:
> include/nand.h:39:6: note: declared here
>  void board_nand_init(void);
>       ^~~~~~~~~~~~~~~
> make[4]: *** [spl/drivers/mtd/nand/mxs_nand_spl.o] Error 1
> make[3]: *** [spl/drivers/mtd/nand] Error 2
> make[2]: *** [spl/drivers] Error 2
> make[1]: *** [spl/u-boot-spl] Error 2
> make: *** [sub-make] Error 2
> 
> 
> The second one is a missing include, like in mx28evk:
> 
> 
> drivers/mtd/nand/mxs_nand.c: In function ?mxs_nand_calc_ecc_layout_by_info?:
> drivers/mtd/nand/mxs_nand.c:172:7: error: ?SZ_512? undeclared (first use
> in this function)
>   case SZ_512:
>        ^~~~~~
> drivers/mtd/nand/mxs_nand.c:172:7: note: each undeclared identifier is
> reported only once for each function it appears in
> drivers/mtd/nand/mxs_nand.c:175:7: error: ?SZ_1K? undeclared (first use
> in this function)
>   case SZ_1K:
>        ^~~~~
> make[2]: *** [drivers/mtd/nand/mxs_nand.o] Error 1
> make[1]: *** [drivers/mtd/nand] Error 2
> make: *** [sub-make] Error 2
> 
> Can you take a look ?

Hm, I thought I did some testing across all boards, but maybe not with
all patches applied.

Will look into it.

--
Stefan