[PATCH v3 0/3] nand: omap2: Two and a half improvements

[PATCH v3 0/3] nand: omap2: Two and a half improvements

[Ezequiel Garcia]

Changes from v2:

  * Fixed some silly mistakes in patch 3, and reverted the removed
    error messages when nand_bch_init fails.

Changes from v1:

  * Rebased on v3.14-rc2.

  * Removed a few s/pr_err/dev_err change from patch two, and added it
    to patch three. This was some git-rebase leftover.

Pekon's attempt to add flash BBT support for this driver made me realise
the addition made sense and there were good reasons for it. The first patch
adds support for enabling a flash BBT either from legacy board files or
from devicetree.

While testing this, I noticed how the driver relied on a whole bunch of
horrible #ifdefs, which prevented me from loading the driver as a module.
The second patch attempts to fix that.

The third patch is just a dummy cleanup replacing pr_errs with dev_errs.
This driver is abusing from user messages, but I'm not sure fixing them
worths the trouble.

Ezequiel Garcia (3):
  nand: omap2: Add support for flash-based bad block table
  nand: omap2: Remove horrible ifdefs to fix module probe
  nand: omap2: Replace pr_err with dev_err

 arch/arm/mach-omap2/gpmc.c                   |   2 +
 drivers/mtd/nand/omap2.c                     | 166 +++++++++++++++------------
 include/linux/platform_data/elm.h            |  14 +++
 include/linux/platform_data/mtd-nand-omap2.h |   1 +
 4 files changed, 110 insertions(+), 73 deletions(-)

-- 
2.1.0

[PATCH v3 1/3] nand: omap2: Add support for flash-based bad block table

[Ezequiel Garcia]

This commit adds a new platform-data boolean property that enables use
of a flash-based bad block table. This can also be enabled by setting
the 'nand-on-flash-bbt' devicetree property.

If the flash BBT is not enabled, the driver falls back to use OOB
bad block markers only, as before. If the flash BBT is enabled the
kernel will keep track of bad blocks using a BBT, in addition to
the OOB markers.

As explained by Brian Norris the reasons for using a BBT are:

""
The primary reason would be that NAND datasheets specify it these days.
A better argument is that nobody guarantees that you can write a
bad block marker to a worn out block; you may just get program failures.

This has been acknowledged by several developers over the last several
years.

Additionally, you get a boot-time performance improvement if you only
have to read a few pages, instead of a page or two from every block on
the flash.
""

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 arch/arm/mach-omap2/gpmc.c                   | 2 ++
 drivers/mtd/nand/omap2.c                     | 6 +++++-
 include/linux/platform_data/mtd-nand-omap2.h | 1 +
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 2f97228..b55a225 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -1440,6 +1440,8 @@ static int gpmc_probe_nand_child(struct platform_device *pdev,
 				break;
 			}
 
+	gpmc_nand_data->flash_bbt = of_get_nand_on_flash_bbt(child);
+
 	val = of_get_nand_bus_width(child);
 	if (val == 16)
 		gpmc_nand_data->devsize = NAND_BUSWIDTH_16;
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 5967b38..e1a9b31 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1663,7 +1663,6 @@ static int omap_nand_probe(struct platform_device *pdev)
 	mtd->owner		= THIS_MODULE;
 	nand_chip		= &info->nand;
 	nand_chip->ecc.priv	= NULL;
-	nand_chip->options	|= NAND_SKIP_BBTSCAN;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	nand_chip->IO_ADDR_R = devm_ioremap_resource(&pdev->dev, res);
@@ -1692,6 +1691,11 @@ static int omap_nand_probe(struct platform_device *pdev)
 		nand_chip->chip_delay = 50;
 	}
 
+	if (pdata->flash_bbt)
+		nand_chip->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
+	else
+		nand_chip->options |= NAND_SKIP_BBTSCAN;
+
 	/* scan NAND device connected to chip controller */
 	nand_chip->options |= pdata->devsize & NAND_BUSWIDTH_16;
 	if (nand_scan_ident(mtd, 1, NULL)) {
diff --git a/include/linux/platform_data/mtd-nand-omap2.h b/include/linux/platform_data/mtd-nand-omap2.h
index 16ec262..090bbab 100644
--- a/include/linux/platform_data/mtd-nand-omap2.h
+++ b/include/linux/platform_data/mtd-nand-omap2.h
@@ -71,6 +71,7 @@ struct omap_nand_platform_data {
 	struct mtd_partition	*parts;
 	int			nr_parts;
 	bool			dev_ready;
+	bool			flash_bbt;
 	enum nand_io		xfer_type;
 	int			devsize;
 	enum omap_ecc           ecc_opt;
-- 
2.1.0

[PATCH v3 2/3] nand: omap2: Remove horrible ifdefs to fix module probe

[Ezequiel Garcia]

The current code abuses ifdefs to determine if the selected ECC scheme
is supported by the running kernel. As a result the code is hard to read,
and it also fails to load as a module.

This commit removes all the ifdefs and instead introduces a function
omap2_nand_ecc_check() to check if the ECC is supported by using
IS_ENABLED(CONFIG_xxx).

Since IS_ENABLED() is true when a config is =y or =m, this change fixes the
module so it can be loaded with no issues.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 drivers/mtd/nand/omap2.c          | 130 +++++++++++++++++++++-----------------
 include/linux/platform_data/elm.h |  14 ++++
 2 files changed, 85 insertions(+), 59 deletions(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index e1a9b31..f97a4ff 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -136,7 +136,6 @@
 
 #define BADBLOCK_MARKER_LENGTH		2
 
-#ifdef CONFIG_MTD_NAND_OMAP_BCH
 static u_char bch16_vector[] = {0xf5, 0x24, 0x1c, 0xd0, 0x61, 0xb3, 0xf1, 0x55,
 				0x2e, 0x2c, 0x86, 0xa3, 0xed, 0x36, 0x1b, 0x78,
 				0x48, 0x76, 0xa9, 0x3b, 0x97, 0xd1, 0x7a, 0x93,
@@ -144,7 +143,6 @@ static u_char bch16_vector[] = {0xf5, 0x24, 0x1c, 0xd0, 0x61, 0xb3, 0xf1, 0x55,
 static u_char bch8_vector[] = {0xf3, 0xdb, 0x14, 0x16, 0x8b, 0xd2, 0xbe, 0xcc,
 	0xac, 0x6b, 0xff, 0x99, 0x7b};
 static u_char bch4_vector[] = {0x00, 0x6b, 0x31, 0xdd, 0x41, 0xbc, 0x10};
-#endif
 
 /* oob info generated runtime depending on ecc algorithm and layout selected */
 static struct nand_ecclayout omap_oobinfo;
@@ -1292,7 +1290,6 @@ static int __maybe_unused omap_calculate_ecc_bch(struct mtd_info *mtd,
 	return 0;
 }
 
-#ifdef CONFIG_MTD_NAND_OMAP_BCH
 /**
  * erased_sector_bitflips - count bit flips
  * @data:	data sector buffer
@@ -1593,33 +1590,71 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip,
 /**
  * is_elm_present - checks for presence of ELM module by scanning DT nodes
  * @omap_nand_info: NAND device structure containing platform data
- * @bch_type: 0x0=BCH4, 0x1=BCH8, 0x2=BCH16
  */
-static int is_elm_present(struct omap_nand_info *info,
-			struct device_node *elm_node, enum bch_ecc bch_type)
+static bool is_elm_present(struct omap_nand_info *info,
+			   struct device_node *elm_node)
 {
 	struct platform_device *pdev;
-	struct nand_ecc_ctrl *ecc = &info->nand.ecc;
-	int err;
+
 	/* check whether elm-id is passed via DT */
 	if (!elm_node) {
 		pr_err("nand: error: ELM DT node not found\n");
-		return -ENODEV;
+		return false;
 	}
 	pdev = of_find_device_by_node(elm_node);
 	/* check whether ELM device is registered */
 	if (!pdev) {
 		pr_err("nand: error: ELM device not found\n");
-		return -ENODEV;
+		return false;
 	}
 	/* ELM module available, now configure it */
 	info->elm_dev = &pdev->dev;
-	err = elm_config(info->elm_dev, bch_type,
-		(info->mtd.writesize / ecc->size), ecc->size, ecc->bytes);
+	return true;
+}
 
-	return err;
+static bool omap2_nand_ecc_check(struct omap_nand_info *info,
+				 struct omap_nand_platform_data	*pdata)
+{
+	bool ecc_needs_bch, ecc_needs_omap_bch, ecc_needs_elm;
+
+	switch (info->ecc_opt) {
+	case OMAP_ECC_BCH4_CODE_HW_DETECTION_SW:
+	case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW:
+		ecc_needs_omap_bch = false;
+		ecc_needs_bch = true;
+		ecc_needs_elm = false;
+		break;
+	case OMAP_ECC_BCH4_CODE_HW:
+	case OMAP_ECC_BCH8_CODE_HW:
+	case OMAP_ECC_BCH16_CODE_HW:
+		ecc_needs_omap_bch = true;
+		ecc_needs_bch = false;
+		ecc_needs_elm = true;
+		break;
+	default:
+		ecc_needs_omap_bch = false;
+		ecc_needs_bch = false;
+		ecc_needs_elm = false;
+		break;
+	}
+
+	if (ecc_needs_bch && !IS_ENABLED(CONFIG_MTD_NAND_ECC_BCH)) {
+		dev_err(&info->pdev->dev,
+			"CONFIG_MTD_NAND_ECC_BCH not enabled\n");
+		return false;
+	}
+	if (ecc_needs_omap_bch && !IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH)) {
+		dev_err(&info->pdev->dev,
+			"CONFIG_MTD_NAND_OMAP_BCH not enabled\n");
+		return false;
+	}
+	if (ecc_needs_elm && !is_elm_present(info, pdata->elm_of_node)) {
+		dev_err(&info->pdev->dev, "ELM not available\n");
+		return false;
+	}
+
+	return true;
 }
-#endif /* CONFIG_MTD_NAND_ECC_BCH */
 
 static int omap_nand_probe(struct platform_device *pdev)
 {
@@ -1797,6 +1832,11 @@ static int omap_nand_probe(struct platform_device *pdev)
 		goto return_error;
 	}
 
+	if (!omap2_nand_ecc_check(info, pdata)) {
+		err = -EINVAL;
+		goto return_error;
+	}
+
 	/* populate MTD interface based on ECC scheme */
 	ecclayout		= &omap_oobinfo;
 	switch (info->ecc_opt) {
@@ -1829,7 +1869,6 @@ static int omap_nand_probe(struct platform_device *pdev)
 		break;
 
 	case OMAP_ECC_BCH4_CODE_HW_DETECTION_SW:
-#ifdef CONFIG_MTD_NAND_ECC_BCH
 		pr_info("nand: using OMAP_ECC_BCH4_CODE_HW_DETECTION_SW\n");
 		nand_chip->ecc.mode		= NAND_ECC_HW;
 		nand_chip->ecc.size		= 512;
@@ -1861,14 +1900,8 @@ static int omap_nand_probe(struct platform_device *pdev)
 			err = -EINVAL;
 		}
 		break;
-#else
-		pr_err("nand: error: CONFIG_MTD_NAND_ECC_BCH not enabled\n");
-		err = -EINVAL;
-		goto return_error;
-#endif
 
 	case OMAP_ECC_BCH4_CODE_HW:
-#ifdef CONFIG_MTD_NAND_OMAP_BCH
 		pr_info("nand: using OMAP_ECC_BCH4_CODE_HW ECC scheme\n");
 		nand_chip->ecc.mode		= NAND_ECC_HW;
 		nand_chip->ecc.size		= 512;
@@ -1890,21 +1923,15 @@ static int omap_nand_probe(struct platform_device *pdev)
 		/* reserved marker already included in ecclayout->eccbytes */
 		ecclayout->oobfree->offset	=
 				ecclayout->eccpos[ecclayout->eccbytes - 1] + 1;
-		/* This ECC scheme requires ELM H/W block */
-		if (is_elm_present(info, pdata->elm_of_node, BCH4_ECC) < 0) {
-			pr_err("nand: error: could not initialize ELM\n");
-			err = -ENODEV;
+
+		err = elm_config(info->elm_dev, BCH4_ECC,
+				 info->mtd.writesize / nand_chip->ecc.size,
+				 nand_chip->ecc.size, nand_chip->ecc.bytes);
+		if (err < 0)
 			goto return_error;
-		}
 		break;
-#else
-		pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n");
-		err = -EINVAL;
-		goto return_error;
-#endif
 
 	case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW:
-#ifdef CONFIG_MTD_NAND_ECC_BCH
 		pr_info("nand: using OMAP_ECC_BCH8_CODE_HW_DETECTION_SW\n");
 		nand_chip->ecc.mode		= NAND_ECC_HW;
 		nand_chip->ecc.size		= 512;
@@ -1937,14 +1964,8 @@ static int omap_nand_probe(struct platform_device *pdev)
 			goto return_error;
 		}
 		break;
-#else
-		pr_err("nand: error: CONFIG_MTD_NAND_ECC_BCH not enabled\n");
-		err = -EINVAL;
-		goto return_error;
-#endif
 
 	case OMAP_ECC_BCH8_CODE_HW:
-#ifdef CONFIG_MTD_NAND_OMAP_BCH
 		pr_info("nand: using OMAP_ECC_BCH8_CODE_HW ECC scheme\n");
 		nand_chip->ecc.mode		= NAND_ECC_HW;
 		nand_chip->ecc.size		= 512;
@@ -1956,12 +1977,13 @@ static int omap_nand_probe(struct platform_device *pdev)
 		nand_chip->ecc.calculate	= omap_calculate_ecc_bch;
 		nand_chip->ecc.read_page	= omap_read_page_bch;
 		nand_chip->ecc.write_page	= omap_write_page_bch;
-		/* This ECC scheme requires ELM H/W block */
-		err = is_elm_present(info, pdata->elm_of_node, BCH8_ECC);
-		if (err < 0) {
-			pr_err("nand: error: could not initialize ELM\n");
+
+		err = elm_config(info->elm_dev, BCH8_ECC,
+				 info->mtd.writesize / nand_chip->ecc.size,
+				 nand_chip->ecc.size, nand_chip->ecc.bytes);
+		if (err < 0)
 			goto return_error;
-		}
+
 		/* define ECC layout */
 		ecclayout->eccbytes		= nand_chip->ecc.bytes *
 							(mtd->writesize /
@@ -1973,14 +1995,8 @@ static int omap_nand_probe(struct platform_device *pdev)
 		ecclayout->oobfree->offset	=
 				ecclayout->eccpos[ecclayout->eccbytes - 1] + 1;
 		break;
-#else
-		pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n");
-		err = -EINVAL;
-		goto return_error;
-#endif
 
 	case OMAP_ECC_BCH16_CODE_HW:
-#ifdef CONFIG_MTD_NAND_OMAP_BCH
 		pr_info("using OMAP_ECC_BCH16_CODE_HW ECC scheme\n");
 		nand_chip->ecc.mode		= NAND_ECC_HW;
 		nand_chip->ecc.size		= 512;
@@ -1991,12 +2007,13 @@ static int omap_nand_probe(struct platform_device *pdev)
 		nand_chip->ecc.calculate	= omap_calculate_ecc_bch;
 		nand_chip->ecc.read_page	= omap_read_page_bch;
 		nand_chip->ecc.write_page	= omap_write_page_bch;
-		/* This ECC scheme requires ELM H/W block */
-		err = is_elm_present(info, pdata->elm_of_node, BCH16_ECC);
-		if (err < 0) {
-			pr_err("ELM is required for this ECC scheme\n");
+
+		err = elm_config(info->elm_dev, BCH16_ECC,
+				 info->mtd.writesize / nand_chip->ecc.size,
+				 nand_chip->ecc.size, nand_chip->ecc.bytes);
+		if (err < 0)
 			goto return_error;
-		}
+
 		/* define ECC layout */
 		ecclayout->eccbytes		= nand_chip->ecc.bytes *
 							(mtd->writesize /
@@ -2008,11 +2025,6 @@ static int omap_nand_probe(struct platform_device *pdev)
 		ecclayout->oobfree->offset	=
 				ecclayout->eccpos[ecclayout->eccbytes - 1] + 1;
 		break;
-#else
-		pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n");
-		err = -EINVAL;
-		goto return_error;
-#endif
 	default:
 		pr_err("nand: error: invalid or unsupported ECC scheme\n");
 		err = -EINVAL;
diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h
index 780d1e9..25d1bca 100644
--- a/include/linux/platform_data/elm.h
+++ b/include/linux/platform_data/elm.h
@@ -42,8 +42,22 @@ struct elm_errorvec {
 	int error_loc[16];
 };
 
+#if IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH)
 void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
 		struct elm_errorvec *err_vec);
 int elm_config(struct device *dev, enum bch_ecc bch_type,
 	int ecc_steps, int ecc_step_size, int ecc_syndrome_size);
+#else
+void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
+		struct elm_errorvec *err_vec)
+{
+}
+
+int elm_config(struct device *dev, enum bch_ecc bch_type,
+	int ecc_steps, int ecc_step_size, int ecc_syndrome_size)
+{
+	return -ENOSYS;
+}
+#endif /* CONFIG_MTD_NAND_ECC_BCH */
+
 #endif /* __ELM_H */
-- 
2.1.0

[PATCH v3 3/3] nand: omap2: Replace pr_err with dev_err

[Ezequiel Garcia]

Usage of pr_err is frowned upon, so replace it with dev_err.
Aditionally, the message on nand_bch_init() error is redundant,
since proper error is showed if the function fails.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 drivers/mtd/nand/omap2.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index f97a4ff..3b357e9 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1375,7 +1375,7 @@ static int omap_elm_correct_data(struct mtd_info *mtd, u_char *data,
 		erased_ecc_vec = bch16_vector;
 		break;
 	default:
-		pr_err("invalid driver configuration\n");
+		dev_err(&info->pdev->dev, "invalid driver configuration\n");
 		return -EINVAL;
 	}
 
@@ -1446,7 +1446,8 @@ static int omap_elm_correct_data(struct mtd_info *mtd, u_char *data,
 	err = 0;
 	for (i = 0; i < eccsteps; i++) {
 		if (err_vec[i].error_uncorrectable) {
-			pr_err("nand: uncorrectable bit-flips found\n");
+			dev_err(&info->pdev->dev,
+				"uncorrectable bit-flips found\n");
 			err = -EBADMSG;
 		} else if (err_vec[i].error_reported) {
 			for (j = 0; j < err_vec[i].error_count; j++) {
@@ -1483,8 +1484,9 @@ static int omap_elm_correct_data(struct mtd_info *mtd, u_char *data,
 							1 << bit_pos;
 					}
 				} else {
-					pr_err("invalid bit-flip @ %d:%d\n",
-							 byte_pos, bit_pos);
+					dev_err(&info->pdev->dev,
+						"invalid bit-flip @ %d:%d\n",
+						byte_pos, bit_pos);
 					err = -EBADMSG;
 				}
 			}
@@ -1598,13 +1600,13 @@ static bool is_elm_present(struct omap_nand_info *info,
 
 	/* check whether elm-id is passed via DT */
 	if (!elm_node) {
-		pr_err("nand: error: ELM DT node not found\n");
+		dev_err(&info->pdev->dev, "ELM devicetree node not found\n");
 		return false;
 	}
 	pdev = of_find_device_by_node(elm_node);
 	/* check whether ELM device is registered */
 	if (!pdev) {
-		pr_err("nand: error: ELM device not found\n");
+		dev_err(&info->pdev->dev, "ELM device not found\n");
 		return false;
 	}
 	/* ELM module available, now configure it */
@@ -1734,14 +1736,14 @@ static int omap_nand_probe(struct platform_device *pdev)
 	/* scan NAND device connected to chip controller */
 	nand_chip->options |= pdata->devsize & NAND_BUSWIDTH_16;
 	if (nand_scan_ident(mtd, 1, NULL)) {
-		pr_err("nand device scan failed, may be bus-width mismatch\n");
+		dev_err(&info->pdev->dev, "scan failed, may be bus-width mismatch\n");
 		err = -ENXIO;
 		goto return_error;
 	}
 
 	/* check for small page devices */
 	if ((mtd->oobsize < 64) && (pdata->ecc_opt != OMAP_ECC_HAM1_CODE_HW)) {
-		pr_err("small page devices are not supported\n");
+		dev_err(&info->pdev->dev, "small page devices are not supported\n");
 		err = -EINVAL;
 		goto return_error;
 	}
@@ -1896,8 +1898,9 @@ static int omap_nand_probe(struct platform_device *pdev)
 							nand_chip->ecc.bytes,
 							&ecclayout);
 		if (!nand_chip->ecc.priv) {
-			pr_err("nand: error: unable to use s/w BCH library\n");
+			dev_err(&info->pdev->dev, "unable to use BCH library\n");
 			err = -EINVAL;
+			goto return_error;
 		}
 		break;
 
@@ -1959,7 +1962,7 @@ static int omap_nand_probe(struct platform_device *pdev)
 							nand_chip->ecc.bytes,
 							&ecclayout);
 		if (!nand_chip->ecc.priv) {
-			pr_err("nand: error: unable to use s/w BCH library\n");
+			dev_err(&info->pdev->dev, "unable to use BCH library\n");
 			err = -EINVAL;
 			goto return_error;
 		}
@@ -2026,7 +2029,7 @@ static int omap_nand_probe(struct platform_device *pdev)
 				ecclayout->eccpos[ecclayout->eccbytes - 1] + 1;
 		break;
 	default:
-		pr_err("nand: error: invalid or unsupported ECC scheme\n");
+		dev_err(&info->pdev->dev, "invalid or unsupported ECC scheme\n");
 		err = -EINVAL;
 		goto return_error;
 	}
@@ -2038,8 +2041,9 @@ static int omap_nand_probe(struct platform_device *pdev)
 	ecclayout->oobfree->length = mtd->oobsize - ecclayout->oobfree->offset;
 	/* check if NAND device's OOB is enough to store ECC signatures */
 	if (mtd->oobsize < (ecclayout->eccbytes + BADBLOCK_MARKER_LENGTH)) {
-		pr_err("not enough OOB bytes required = %d, available=%d\n",
-					   ecclayout->eccbytes, mtd->oobsize);
+		dev_err(&info->pdev->dev,
+			"not enough OOB bytes required = %d, available=%d\n",
+			ecclayout->eccbytes, mtd->oobsize);
 		err = -EINVAL;
 		goto return_error;
 	}
-- 
2.1.0

Re: [PATCH v3 1/3] nand: omap2: Add support for flash-based bad block table

[Roger Quadros]

On 09/11/2014 06:02 PM, Ezequiel Garcia wrote:
> This commit adds a new platform-data boolean property that enables use
> of a flash-based bad block table. This can also be enabled by setting
> the 'nand-on-flash-bbt' devicetree property.
> 
> If the flash BBT is not enabled, the driver falls back to use OOB
> bad block markers only, as before. If the flash BBT is enabled the
> kernel will keep track of bad blocks using a BBT, in addition to
> the OOB markers.
> 
> As explained by Brian Norris the reasons for using a BBT are:
> 
> ""
> The primary reason would be that NAND datasheets specify it these days.
> A better argument is that nobody guarantees that you can write a
> bad block marker to a worn out block; you may just get program failures.
> 
> This has been acknowledged by several developers over the last several
> years.
> 
> Additionally, you get a boot-time performance improvement if you only
> have to read a few pages, instead of a page or two from every block on
> the flash.
> ""
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

Acked-by: Roger Quadros <rogerq@ti.com>

cheers,
-roger

Re: [PATCH v3 2/3] nand: omap2: Remove horrible ifdefs to fix module probe

[Roger Quadros]

On 09/11/2014 06:02 PM, Ezequiel Garcia wrote:
> The current code abuses ifdefs to determine if the selected ECC scheme
> is supported by the running kernel. As a result the code is hard to read,
> and it also fails to load as a module.
> 
> This commit removes all the ifdefs and instead introduces a function
> omap2_nand_ecc_check() to check if the ECC is supported by using
> IS_ENABLED(CONFIG_xxx).
> 
> Since IS_ENABLED() is true when a config is =y or =m, this change fixes the
> module so it can be loaded with no issues.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

Acked-by: Roger Quadros <rogerq@ti.com>

cheers,
-roger

Re: [PATCH v3 3/3] nand: omap2: Replace pr_err with dev_err

[Roger Quadros]

On 09/11/2014 06:02 PM, Ezequiel Garcia wrote:
> Usage of pr_err is frowned upon, so replace it with dev_err.
> Aditionally, the message on nand_bch_init() error is redundant,
> since proper error is showed if the function fails.

You could have omitted the details about nand_bch_init() in the commit
message.

> 
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

Acked-by: Roger Quadros <rogerq@ti.com>

cheers,
-roger

Re: [PATCH v3 1/3] nand: omap2: Add support for flash-based bad block table

[Brian Norris]

On Thu, Sep 11, 2014 at 12:02:08PM -0300, Ezequiel Garcia wrote:
> This commit adds a new platform-data boolean property that enables use
> of a flash-based bad block table. This can also be enabled by setting
> the 'nand-on-flash-bbt' devicetree property.
> 
> If the flash BBT is not enabled, the driver falls back to use OOB
> bad block markers only, as before. If the flash BBT is enabled the
> kernel will keep track of bad blocks using a BBT, in addition to
> the OOB markers.
> 
> As explained by Brian Norris the reasons for using a BBT are:
> 
> ""
> The primary reason would be that NAND datasheets specify it these days.
> A better argument is that nobody guarantees that you can write a
> bad block marker to a worn out block; you may just get program failures.
> 
> This has been acknowledged by several developers over the last several
> years.
> 
> Additionally, you get a boot-time performance improvement if you only
> have to read a few pages, instead of a page or two from every block on
> the flash.
> ""
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

Pushed this one to l2-mtd.git. Thanks!

But I do have one question below, and I have comments for patch 2.

> ---
>  arch/arm/mach-omap2/gpmc.c                   | 2 ++
>  drivers/mtd/nand/omap2.c                     | 6 +++++-
>  include/linux/platform_data/mtd-nand-omap2.h | 1 +
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 2f97228..b55a225 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -1440,6 +1440,8 @@ static int gpmc_probe_nand_child(struct platform_device *pdev,
>  				break;
>  			}
>  
> +	gpmc_nand_data->flash_bbt = of_get_nand_on_flash_bbt(child);
> +
>  	val = of_get_nand_bus_width(child);
>  	if (val == 16)
>  		gpmc_nand_data->devsize = NAND_BUSWIDTH_16;
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 5967b38..e1a9b31 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -1663,7 +1663,6 @@ static int omap_nand_probe(struct platform_device *pdev)
>  	mtd->owner		= THIS_MODULE;
>  	nand_chip		= &info->nand;
>  	nand_chip->ecc.priv	= NULL;
> -	nand_chip->options	|= NAND_SKIP_BBTSCAN;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	nand_chip->IO_ADDR_R = devm_ioremap_resource(&pdev->dev, res);
> @@ -1692,6 +1691,11 @@ static int omap_nand_probe(struct platform_device *pdev)
>  		nand_chip->chip_delay = 50;
>  	}
>  
> +	if (pdata->flash_bbt)
> +		nand_chip->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
> +	else
> +		nand_chip->options |= NAND_SKIP_BBTSCAN;

Can you remind me: why do you use SKIP_BBTSCAN? Doesn't that mean you're
skipping all boot-time scanning for bad blocks, and resorting to
on-demand scanning (chip->block_bad()) every time you need to check for
bad blocks?

> +
>  	/* scan NAND device connected to chip controller */
>  	nand_chip->options |= pdata->devsize & NAND_BUSWIDTH_16;
>  	if (nand_scan_ident(mtd, 1, NULL)) {
> diff --git a/include/linux/platform_data/mtd-nand-omap2.h b/include/linux/platform_data/mtd-nand-omap2.h
> index 16ec262..090bbab 100644
> --- a/include/linux/platform_data/mtd-nand-omap2.h
> +++ b/include/linux/platform_data/mtd-nand-omap2.h
> @@ -71,6 +71,7 @@ struct omap_nand_platform_data {
>  	struct mtd_partition	*parts;
>  	int			nr_parts;
>  	bool			dev_ready;
> +	bool			flash_bbt;
>  	enum nand_io		xfer_type;
>  	int			devsize;
>  	enum omap_ecc           ecc_opt;

Brian

Re: [PATCH v3 1/3] nand: omap2: Add support for flash-based bad block table

[Brian Norris]

On Thu, Sep 11, 2014 at 12:02:08PM -0300, Ezequiel Garcia wrote:
> This commit adds a new platform-data boolean property that enables use
> of a flash-based bad block table. This can also be enabled by setting
> the 'nand-on-flash-bbt' devicetree property.
> 
> If the flash BBT is not enabled, the driver falls back to use OOB
> bad block markers only, as before. If the flash BBT is enabled the
> kernel will keep track of bad blocks using a BBT, in addition to
> the OOB markers.
> 
> As explained by Brian Norris the reasons for using a BBT are:
> 
> ""
> The primary reason would be that NAND datasheets specify it these days.
> A better argument is that nobody guarantees that you can write a
> bad block marker to a worn out block; you may just get program failures.
> 
> This has been acknowledged by several developers over the last several
> years.
> 
> Additionally, you get a boot-time performance improvement if you only
> have to read a few pages, instead of a page or two from every block on
> the flash.
> ""
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

Pushed this one to l2-mtd.git. Thanks!

But I do have one question below, and I have comments for patch 2.

> ---
>  arch/arm/mach-omap2/gpmc.c                   | 2 ++
>  drivers/mtd/nand/omap2.c                     | 6 +++++-
>  include/linux/platform_data/mtd-nand-omap2.h | 1 +
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 2f97228..b55a225 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -1440,6 +1440,8 @@ static int gpmc_probe_nand_child(struct platform_device *pdev,
>  				break;
>  			}
>  
> +	gpmc_nand_data->flash_bbt = of_get_nand_on_flash_bbt(child);
> +
>  	val = of_get_nand_bus_width(child);
>  	if (val == 16)
>  		gpmc_nand_data->devsize = NAND_BUSWIDTH_16;
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 5967b38..e1a9b31 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -1663,7 +1663,6 @@ static int omap_nand_probe(struct platform_device *pdev)
>  	mtd->owner		= THIS_MODULE;
>  	nand_chip		= &info->nand;
>  	nand_chip->ecc.priv	= NULL;
> -	nand_chip->options	|= NAND_SKIP_BBTSCAN;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	nand_chip->IO_ADDR_R = devm_ioremap_resource(&pdev->dev, res);
> @@ -1692,6 +1691,11 @@ static int omap_nand_probe(struct platform_device *pdev)
>  		nand_chip->chip_delay = 50;
>  	}
>  
> +	if (pdata->flash_bbt)
> +		nand_chip->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
> +	else
> +		nand_chip->options |= NAND_SKIP_BBTSCAN;

Can you remind me: why do you use SKIP_BBTSCAN? Doesn't that mean you're
skipping all boot-time scanning for bad blocks, and resorting to
on-demand scanning (chip->block_bad()) every time you need to check for
bad blocks?

> +
>  	/* scan NAND device connected to chip controller */
>  	nand_chip->options |= pdata->devsize & NAND_BUSWIDTH_16;
>  	if (nand_scan_ident(mtd, 1, NULL)) {
> diff --git a/include/linux/platform_data/mtd-nand-omap2.h b/include/linux/platform_data/mtd-nand-omap2.h
> index 16ec262..090bbab 100644
> --- a/include/linux/platform_data/mtd-nand-omap2.h
> +++ b/include/linux/platform_data/mtd-nand-omap2.h
> @@ -71,6 +71,7 @@ struct omap_nand_platform_data {
>  	struct mtd_partition	*parts;
>  	int			nr_parts;
>  	bool			dev_ready;
> +	bool			flash_bbt;
>  	enum nand_io		xfer_type;
>  	int			devsize;
>  	enum omap_ecc           ecc_opt;

Brian

Re: [PATCH v3 2/3] nand: omap2: Remove horrible ifdefs to fix module probe

[Brian Norris]

On Thu, Sep 11, 2014 at 12:02:09PM -0300, Ezequiel Garcia wrote:
> The current code abuses ifdefs to determine if the selected ECC scheme
> is supported by the running kernel. As a result the code is hard to read,
> and it also fails to load as a module.

Yes, a real eyesore, with little benefit from the #ifdef's.

> This commit removes all the ifdefs and instead introduces a function
> omap2_nand_ecc_check() to check if the ECC is supported by using
> IS_ENABLED(CONFIG_xxx).
> 
> Since IS_ENABLED() is true when a config is =y or =m, this change fixes the
> module so it can be loaded with no issues.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
[...]
> diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h
> index 780d1e9..25d1bca 100644
> --- a/include/linux/platform_data/elm.h
> +++ b/include/linux/platform_data/elm.h
> @@ -42,8 +42,22 @@ struct elm_errorvec {
>  	int error_loc[16];
>  };
>  
> +#if IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH)
>  void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
>  		struct elm_errorvec *err_vec);
>  int elm_config(struct device *dev, enum bch_ecc bch_type,
>  	int ecc_steps, int ecc_step_size, int ecc_syndrome_size);
> +#else
> +void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
> +		struct elm_errorvec *err_vec)

You should not be defining non-static functions in a header. Now these
functions will be uselessly placed in every compilation unit that
includes this header. You should probably make this function 'static
inline'.

> +{
> +}
> +
> +int elm_config(struct device *dev, enum bch_ecc bch_type,
> +	int ecc_steps, int ecc_step_size, int ecc_syndrome_size)

And same here.

> +{
> +	return -ENOSYS;
> +}
> +#endif /* CONFIG_MTD_NAND_ECC_BCH */
> +
>  #endif /* __ELM_H */

Brian

Re: [PATCH v3 2/3] nand: omap2: Remove horrible ifdefs to fix module probe

[Brian Norris]

On Thu, Sep 11, 2014 at 12:02:09PM -0300, Ezequiel Garcia wrote:
> The current code abuses ifdefs to determine if the selected ECC scheme
> is supported by the running kernel. As a result the code is hard to read,
> and it also fails to load as a module.

Yes, a real eyesore, with little benefit from the #ifdef's.

> This commit removes all the ifdefs and instead introduces a function
> omap2_nand_ecc_check() to check if the ECC is supported by using
> IS_ENABLED(CONFIG_xxx).
> 
> Since IS_ENABLED() is true when a config is =y or =m, this change fixes the
> module so it can be loaded with no issues.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
[...]
> diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h
> index 780d1e9..25d1bca 100644
> --- a/include/linux/platform_data/elm.h
> +++ b/include/linux/platform_data/elm.h
> @@ -42,8 +42,22 @@ struct elm_errorvec {
>  	int error_loc[16];
>  };
>  
> +#if IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH)
>  void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
>  		struct elm_errorvec *err_vec);
>  int elm_config(struct device *dev, enum bch_ecc bch_type,
>  	int ecc_steps, int ecc_step_size, int ecc_syndrome_size);
> +#else
> +void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
> +		struct elm_errorvec *err_vec)

You should not be defining non-static functions in a header. Now these
functions will be uselessly placed in every compilation unit that
includes this header. You should probably make this function 'static
inline'.

> +{
> +}
> +
> +int elm_config(struct device *dev, enum bch_ecc bch_type,
> +	int ecc_steps, int ecc_step_size, int ecc_syndrome_size)

And same here.

> +{
> +	return -ENOSYS;
> +}
> +#endif /* CONFIG_MTD_NAND_ECC_BCH */
> +
>  #endif /* __ELM_H */

Brian

Re: [PATCH v3 1/3] nand: omap2: Add support for flash-based bad block table

[Ezequiel Garcia]

On 18 September 2014 06:59, Brian Norris <computersforpeace@gmail.com> wrote:
> On Thu, Sep 11, 2014 at 12:02:08PM -0300, Ezequiel Garcia wrote:
>> This commit adds a new platform-data boolean property that enables use
>> of a flash-based bad block table. This can also be enabled by setting
>> the 'nand-on-flash-bbt' devicetree property.
>>
>> If the flash BBT is not enabled, the driver falls back to use OOB
>> bad block markers only, as before. If the flash BBT is enabled the
>> kernel will keep track of bad blocks using a BBT, in addition to
>> the OOB markers.
>>
>> As explained by Brian Norris the reasons for using a BBT are:
>>
>> ""
>> The primary reason would be that NAND datasheets specify it these days.
>> A better argument is that nobody guarantees that you can write a
>> bad block marker to a worn out block; you may just get program failures.
>>
>> This has been acknowledged by several developers over the last several
>> years.
>>
>> Additionally, you get a boot-time performance improvement if you only
>> have to read a few pages, instead of a page or two from every block on
>> the flash.
>> ""
>>
>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>
> Pushed this one to l2-mtd.git. Thanks!
>
> But I do have one question below, and I have comments for patch 2.
>
>> ---
>>  arch/arm/mach-omap2/gpmc.c                   | 2 ++
>>  drivers/mtd/nand/omap2.c                     | 6 +++++-
>>  include/linux/platform_data/mtd-nand-omap2.h | 1 +
>>  3 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
>> index 2f97228..b55a225 100644
>> --- a/arch/arm/mach-omap2/gpmc.c
>> +++ b/arch/arm/mach-omap2/gpmc.c
>> @@ -1440,6 +1440,8 @@ static int gpmc_probe_nand_child(struct platform_device *pdev,
>>                               break;
>>                       }
>>
>> +     gpmc_nand_data->flash_bbt = of_get_nand_on_flash_bbt(child);
>> +
>>       val = of_get_nand_bus_width(child);
>>       if (val == 16)
>>               gpmc_nand_data->devsize = NAND_BUSWIDTH_16;
>> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
>> index 5967b38..e1a9b31 100644
>> --- a/drivers/mtd/nand/omap2.c
>> +++ b/drivers/mtd/nand/omap2.c
>> @@ -1663,7 +1663,6 @@ static int omap_nand_probe(struct platform_device *pdev)
>>       mtd->owner              = THIS_MODULE;
>>       nand_chip               = &info->nand;
>>       nand_chip->ecc.priv     = NULL;
>> -     nand_chip->options      |= NAND_SKIP_BBTSCAN;
>>
>>       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>       nand_chip->IO_ADDR_R = devm_ioremap_resource(&pdev->dev, res);
>> @@ -1692,6 +1691,11 @@ static int omap_nand_probe(struct platform_device *pdev)
>>               nand_chip->chip_delay = 50;
>>       }
>>
>> +     if (pdata->flash_bbt)
>> +             nand_chip->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
>> +     else
>> +             nand_chip->options |= NAND_SKIP_BBTSCAN;
>
> Can you remind me: why do you use SKIP_BBTSCAN? Doesn't that mean you're
> skipping all boot-time scanning for bad blocks, and resorting to
> on-demand scanning (chip->block_bad()) every time you need to check for
> bad blocks?
>

Honestly, I have *no* idea, I just retained the previous flag, so to
keep the exact same behavior.

Roger, any ideas? If I have to guess, I'd say this is an attempt to
save some boot time.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/3] nand: omap2: Add support for flash-based bad block table

[Ezequiel Garcia]

On 18 September 2014 06:59, Brian Norris <computersforpeace@gmail.com> wrote:
> On Thu, Sep 11, 2014 at 12:02:08PM -0300, Ezequiel Garcia wrote:
>> This commit adds a new platform-data boolean property that enables use
>> of a flash-based bad block table. This can also be enabled by setting
>> the 'nand-on-flash-bbt' devicetree property.
>>
>> If the flash BBT is not enabled, the driver falls back to use OOB
>> bad block markers only, as before. If the flash BBT is enabled the
>> kernel will keep track of bad blocks using a BBT, in addition to
>> the OOB markers.
>>
>> As explained by Brian Norris the reasons for using a BBT are:
>>
>> ""
>> The primary reason would be that NAND datasheets specify it these days.
>> A better argument is that nobody guarantees that you can write a
>> bad block marker to a worn out block; you may just get program failures.
>>
>> This has been acknowledged by several developers over the last several
>> years.
>>
>> Additionally, you get a boot-time performance improvement if you only
>> have to read a few pages, instead of a page or two from every block on
>> the flash.
>> ""
>>
>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>
> Pushed this one to l2-mtd.git. Thanks!
>
> But I do have one question below, and I have comments for patch 2.
>
>> ---
>>  arch/arm/mach-omap2/gpmc.c                   | 2 ++
>>  drivers/mtd/nand/omap2.c                     | 6 +++++-
>>  include/linux/platform_data/mtd-nand-omap2.h | 1 +
>>  3 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
>> index 2f97228..b55a225 100644
>> --- a/arch/arm/mach-omap2/gpmc.c
>> +++ b/arch/arm/mach-omap2/gpmc.c
>> @@ -1440,6 +1440,8 @@ static int gpmc_probe_nand_child(struct platform_device *pdev,
>>                               break;
>>                       }
>>
>> +     gpmc_nand_data->flash_bbt = of_get_nand_on_flash_bbt(child);
>> +
>>       val = of_get_nand_bus_width(child);
>>       if (val == 16)
>>               gpmc_nand_data->devsize = NAND_BUSWIDTH_16;
>> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
>> index 5967b38..e1a9b31 100644
>> --- a/drivers/mtd/nand/omap2.c
>> +++ b/drivers/mtd/nand/omap2.c
>> @@ -1663,7 +1663,6 @@ static int omap_nand_probe(struct platform_device *pdev)
>>       mtd->owner              = THIS_MODULE;
>>       nand_chip               = &info->nand;
>>       nand_chip->ecc.priv     = NULL;
>> -     nand_chip->options      |= NAND_SKIP_BBTSCAN;
>>
>>       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>       nand_chip->IO_ADDR_R = devm_ioremap_resource(&pdev->dev, res);
>> @@ -1692,6 +1691,11 @@ static int omap_nand_probe(struct platform_device *pdev)
>>               nand_chip->chip_delay = 50;
>>       }
>>
>> +     if (pdata->flash_bbt)
>> +             nand_chip->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
>> +     else
>> +             nand_chip->options |= NAND_SKIP_BBTSCAN;
>
> Can you remind me: why do you use SKIP_BBTSCAN? Doesn't that mean you're
> skipping all boot-time scanning for bad blocks, and resorting to
> on-demand scanning (chip->block_bad()) every time you need to check for
> bad blocks?
>

Honestly, I have *no* idea, I just retained the previous flag, so to
keep the exact same behavior.

Roger, any ideas? If I have to guess, I'd say this is an attempt to
save some boot time.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH v3 2/3] nand: omap2: Remove horrible ifdefs to fix module probe

[Ezequiel Garcia]

On 18 September 2014 07:03, Brian Norris <computersforpeace@gmail.com> wrote:
> On Thu, Sep 11, 2014 at 12:02:09PM -0300, Ezequiel Garcia wrote:
>> The current code abuses ifdefs to determine if the selected ECC scheme
>> is supported by the running kernel. As a result the code is hard to read,
>> and it also fails to load as a module.
>
> Yes, a real eyesore, with little benefit from the #ifdef's.
>
>> This commit removes all the ifdefs and instead introduces a function
>> omap2_nand_ecc_check() to check if the ECC is supported by using
>> IS_ENABLED(CONFIG_xxx).
>>
>> Since IS_ENABLED() is true when a config is =y or =m, this change fixes the
>> module so it can be loaded with no issues.
>>
>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> [...]
>> diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h
>> index 780d1e9..25d1bca 100644
>> --- a/include/linux/platform_data/elm.h
>> +++ b/include/linux/platform_data/elm.h
>> @@ -42,8 +42,22 @@ struct elm_errorvec {
>>       int error_loc[16];
>>  };
>>
>> +#if IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH)
>>  void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
>>               struct elm_errorvec *err_vec);
>>  int elm_config(struct device *dev, enum bch_ecc bch_type,
>>       int ecc_steps, int ecc_step_size, int ecc_syndrome_size);
>> +#else
>> +void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
>> +             struct elm_errorvec *err_vec)
>
> You should not be defining non-static functions in a header. Now these
> functions will be uselessly placed in every compilation unit that
> includes this header. You should probably make this function 'static
> inline'.
>

Darn, of course they must be static! Thanks for spotting this.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] nand: omap2: Remove horrible ifdefs to fix module probe

[Ezequiel Garcia]

On 18 September 2014 07:03, Brian Norris <computersforpeace@gmail.com> wrote:
> On Thu, Sep 11, 2014 at 12:02:09PM -0300, Ezequiel Garcia wrote:
>> The current code abuses ifdefs to determine if the selected ECC scheme
>> is supported by the running kernel. As a result the code is hard to read,
>> and it also fails to load as a module.
>
> Yes, a real eyesore, with little benefit from the #ifdef's.
>
>> This commit removes all the ifdefs and instead introduces a function
>> omap2_nand_ecc_check() to check if the ECC is supported by using
>> IS_ENABLED(CONFIG_xxx).
>>
>> Since IS_ENABLED() is true when a config is =y or =m, this change fixes the
>> module so it can be loaded with no issues.
>>
>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> [...]
>> diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h
>> index 780d1e9..25d1bca 100644
>> --- a/include/linux/platform_data/elm.h
>> +++ b/include/linux/platform_data/elm.h
>> @@ -42,8 +42,22 @@ struct elm_errorvec {
>>       int error_loc[16];
>>  };
>>
>> +#if IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH)
>>  void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
>>               struct elm_errorvec *err_vec);
>>  int elm_config(struct device *dev, enum bch_ecc bch_type,
>>       int ecc_steps, int ecc_step_size, int ecc_syndrome_size);
>> +#else
>> +void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
>> +             struct elm_errorvec *err_vec)
>
> You should not be defining non-static functions in a header. Now these
> functions will be uselessly placed in every compilation unit that
> includes this header. You should probably make this function 'static
> inline'.
>

Darn, of course they must be static! Thanks for spotting this.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH v3 1/3] nand: omap2: Add support for flash-based bad block table

[Roger Quadros]

On 09/18/2014 10:46 AM, Ezequiel Garcia wrote:
> On 18 September 2014 06:59, Brian Norris <computersforpeace@gmail.com> wrote:
>> On Thu, Sep 11, 2014 at 12:02:08PM -0300, Ezequiel Garcia wrote:
>>> This commit adds a new platform-data boolean property that enables use
>>> of a flash-based bad block table. This can also be enabled by setting
>>> the 'nand-on-flash-bbt' devicetree property.
>>>
>>> If the flash BBT is not enabled, the driver falls back to use OOB
>>> bad block markers only, as before. If the flash BBT is enabled the
>>> kernel will keep track of bad blocks using a BBT, in addition to
>>> the OOB markers.
>>>
>>> As explained by Brian Norris the reasons for using a BBT are:
>>>
>>> ""
>>> The primary reason would be that NAND datasheets specify it these days.
>>> A better argument is that nobody guarantees that you can write a
>>> bad block marker to a worn out block; you may just get program failures.
>>>
>>> This has been acknowledged by several developers over the last several
>>> years.
>>>
>>> Additionally, you get a boot-time performance improvement if you only
>>> have to read a few pages, instead of a page or two from every block on
>>> the flash.
>>> ""
>>>
>>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>>
>> Pushed this one to l2-mtd.git. Thanks!
>>
>> But I do have one question below, and I have comments for patch 2.
>>
>>> ---
>>>  arch/arm/mach-omap2/gpmc.c                   | 2 ++
>>>  drivers/mtd/nand/omap2.c                     | 6 +++++-
>>>  include/linux/platform_data/mtd-nand-omap2.h | 1 +
>>>  3 files changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
>>> index 2f97228..b55a225 100644
>>> --- a/arch/arm/mach-omap2/gpmc.c
>>> +++ b/arch/arm/mach-omap2/gpmc.c
>>> @@ -1440,6 +1440,8 @@ static int gpmc_probe_nand_child(struct platform_device *pdev,
>>>                               break;
>>>                       }
>>>
>>> +     gpmc_nand_data->flash_bbt = of_get_nand_on_flash_bbt(child);
>>> +
>>>       val = of_get_nand_bus_width(child);
>>>       if (val == 16)
>>>               gpmc_nand_data->devsize = NAND_BUSWIDTH_16;
>>> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
>>> index 5967b38..e1a9b31 100644
>>> --- a/drivers/mtd/nand/omap2.c
>>> +++ b/drivers/mtd/nand/omap2.c
>>> @@ -1663,7 +1663,6 @@ static int omap_nand_probe(struct platform_device *pdev)
>>>       mtd->owner              = THIS_MODULE;
>>>       nand_chip               = &info->nand;
>>>       nand_chip->ecc.priv     = NULL;
>>> -     nand_chip->options      |= NAND_SKIP_BBTSCAN;
>>>
>>>       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>       nand_chip->IO_ADDR_R = devm_ioremap_resource(&pdev->dev, res);
>>> @@ -1692,6 +1691,11 @@ static int omap_nand_probe(struct platform_device *pdev)
>>>               nand_chip->chip_delay = 50;
>>>       }
>>>
>>> +     if (pdata->flash_bbt)
>>> +             nand_chip->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
>>> +     else
>>> +             nand_chip->options |= NAND_SKIP_BBTSCAN;
>>
>> Can you remind me: why do you use SKIP_BBTSCAN? Doesn't that mean you're
>> skipping all boot-time scanning for bad blocks, and resorting to
>> on-demand scanning (chip->block_bad()) every time you need to check for
>> bad blocks?
>>
> 
> Honestly, I have *no* idea, I just retained the previous flag, so to
> keep the exact same behavior.
> 
> Roger, any ideas? If I have to guess, I'd say this is an attempt to
> save some boot time.
> 
The SKIP_BBTSCAN has been there ever since this driver was introduced in 2009.
by commit 67ce04bf2746.
I'm not sure why it is preferred.

cheers,
-roger

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v3 1/3] nand: omap2: Add support for flash-based bad block table

[Roger Quadros]

On 09/18/2014 10:46 AM, Ezequiel Garcia wrote:
> On 18 September 2014 06:59, Brian Norris <computersforpeace@gmail.com> wrote:
>> On Thu, Sep 11, 2014 at 12:02:08PM -0300, Ezequiel Garcia wrote:
>>> This commit adds a new platform-data boolean property that enables use
>>> of a flash-based bad block table. This can also be enabled by setting
>>> the 'nand-on-flash-bbt' devicetree property.
>>>
>>> If the flash BBT is not enabled, the driver falls back to use OOB
>>> bad block markers only, as before. If the flash BBT is enabled the
>>> kernel will keep track of bad blocks using a BBT, in addition to
>>> the OOB markers.
>>>
>>> As explained by Brian Norris the reasons for using a BBT are:
>>>
>>> ""
>>> The primary reason would be that NAND datasheets specify it these days.
>>> A better argument is that nobody guarantees that you can write a
>>> bad block marker to a worn out block; you may just get program failures.
>>>
>>> This has been acknowledged by several developers over the last several
>>> years.
>>>
>>> Additionally, you get a boot-time performance improvement if you only
>>> have to read a few pages, instead of a page or two from every block on
>>> the flash.
>>> ""
>>>
>>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>>
>> Pushed this one to l2-mtd.git. Thanks!
>>
>> But I do have one question below, and I have comments for patch 2.
>>
>>> ---
>>>  arch/arm/mach-omap2/gpmc.c                   | 2 ++
>>>  drivers/mtd/nand/omap2.c                     | 6 +++++-
>>>  include/linux/platform_data/mtd-nand-omap2.h | 1 +
>>>  3 files changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
>>> index 2f97228..b55a225 100644
>>> --- a/arch/arm/mach-omap2/gpmc.c
>>> +++ b/arch/arm/mach-omap2/gpmc.c
>>> @@ -1440,6 +1440,8 @@ static int gpmc_probe_nand_child(struct platform_device *pdev,
>>>                               break;
>>>                       }
>>>
>>> +     gpmc_nand_data->flash_bbt = of_get_nand_on_flash_bbt(child);
>>> +
>>>       val = of_get_nand_bus_width(child);
>>>       if (val == 16)
>>>               gpmc_nand_data->devsize = NAND_BUSWIDTH_16;
>>> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
>>> index 5967b38..e1a9b31 100644
>>> --- a/drivers/mtd/nand/omap2.c
>>> +++ b/drivers/mtd/nand/omap2.c
>>> @@ -1663,7 +1663,6 @@ static int omap_nand_probe(struct platform_device *pdev)
>>>       mtd->owner              = THIS_MODULE;
>>>       nand_chip               = &info->nand;
>>>       nand_chip->ecc.priv     = NULL;
>>> -     nand_chip->options      |= NAND_SKIP_BBTSCAN;
>>>
>>>       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>       nand_chip->IO_ADDR_R = devm_ioremap_resource(&pdev->dev, res);
>>> @@ -1692,6 +1691,11 @@ static int omap_nand_probe(struct platform_device *pdev)
>>>               nand_chip->chip_delay = 50;
>>>       }
>>>
>>> +     if (pdata->flash_bbt)
>>> +             nand_chip->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
>>> +     else
>>> +             nand_chip->options |= NAND_SKIP_BBTSCAN;
>>
>> Can you remind me: why do you use SKIP_BBTSCAN? Doesn't that mean you're
>> skipping all boot-time scanning for bad blocks, and resorting to
>> on-demand scanning (chip->block_bad()) every time you need to check for
>> bad blocks?
>>
> 
> Honestly, I have *no* idea, I just retained the previous flag, so to
> keep the exact same behavior.
> 
> Roger, any ideas? If I have to guess, I'd say this is an attempt to
> save some boot time.
> 
The SKIP_BBTSCAN has been there ever since this driver was introduced in 2009.
by commit 67ce04bf2746.
I'm not sure why it is preferred.

cheers,
-roger