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

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

[Ezequiel Garcia]

Hi Brian, Roger:

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.

Roger, do you think you can test patches 1 and 2 with different ECCs
and configurations? It's an invasive patch and I don't want to see more
regressions with this driver.

And speaking of modules, the driver loads as "modprobe omap2". And it's not
the only one with a clumsy name: "modprobe elm". I guess we cannot fix it now,
but it would be great to be more careful with driver naming in the future.

Ezequiel Garcia (3):
  nand: omap2: Add support for a flash-based bad block table
  nand: omap2: Refactor the code to remove horrible ifdefs
  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, 108 insertions(+), 75 deletions(-)

-- 
2.0.1

[PATCH 1/3] nand: omap2: Add support for a 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 8bc1338..bafe190 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -1438,6 +1438,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 f0ed92e..1170389 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 660c029..781e9e6 100644
--- a/include/linux/platform_data/mtd-nand-omap2.h
+++ b/include/linux/platform_data/mtd-nand-omap2.h
@@ -62,6 +62,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.0.1

[PATCH 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          | 134 +++++++++++++++++++++-----------------
 include/linux/platform_data/elm.h |  14 ++++
 2 files changed, 87 insertions(+), 61 deletions(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 1170389..987dd94 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;
+		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");
-		return -ENODEV;
+		dev_err(&info->pdev->dev, "ELM device not found\n");
+		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 */
 	nand_chip->ecc.layout	= &omap_oobinfo;
 	ecclayout		= &omap_oobinfo;
@@ -1826,7 +1866,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;
@@ -1858,14 +1897,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;
@@ -1887,21 +1920,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;
@@ -1934,14 +1961,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;
@@ -1953,12 +1974,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 /
@@ -1970,14 +1992,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;
@@ -1988,12 +2004,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 /
@@ -2005,11 +2022,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.0.1

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

[pekon]

+ linux-omap, as this patch touches arch/arm/mach-omap2/gpmc.c

Hi Ezequiel,

On Sunday 07 September 2014 01:26 AM, 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).
>
Yup, true.
Thanks for cleaning up OMAP2 NAND driver, but few feedbacks.

Just for record, current version of omap2.c NAND driver has so
many #ifdef because:

(1) Many ECC schemes like HAM1 and BCH4_xx, BCH8_SW are supported
only for compatibility with old platforms like OMAP3. And these legacy
schemes have significant amount of code and data foot-print
(as compared to whole driver), so it does not make sense to keep
driver bulky for all new platforms which do not use legacy ECC schemes.

(2) Specifically, BCH4_SW and BCH8_SW ECC schemes used software library
/lib/bch.c "directly". Due to which if BCH4_SW or BCH8_SW was include
in code, /lib/bch.c was included implicitly.

Later from following following commit nand_bch.c wrapper was used which
took care to exclude /lib/bch.c when CONFIG_MTD_NAND_ECC_BCH was not
defined, but #ifdef continued :-( (My bad, I accept ..)

	commit 32d42a855a6f1445373f3d680e9125344aca294c
	mtd: nand: omap: use drivers/mtd/nand/nand_bch.c wrapper for BCH ECC 
instead of lib/bch.c


> 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          | 134 +++++++++++++++++++++-----------------
>   include/linux/platform_data/elm.h |  14 ++++
>   2 files changed, 87 insertions(+), 61 deletions(-)

[...]

> @@ -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;
> +		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");
> -		return -ENODEV;
> +		dev_err(&info->pdev->dev, "ELM device not found\n");
> +		return false;
>   	}

Thanks, for this clean-up of replacing pr_err() to dev_err().
Just that if you can resend this as separate patch then I'll surely Ack it.

[...]

> +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;
>   }

Actually this new function is not required.
We already have this check in GPMC driver when we select the ecc-scheme.
Plz refer: File: /arch/arm/mach-omap2/gpmc.c
	@@ static int gpmc_probe_nand_child(...)
	...
	/* select ecc-scheme for NAND */

is_elm_enabled() is a misnomer here. It actually does not check
for ELM DTS node, because that check is already done in GPMC driver.
It rather checks if ELM device is correctly binded and driver
"initialized" before NAND driver is probed.

Though ELM is not required during NAND probe because there is no error
checking/correction required during NAND probe, but this again comes
from the history of maintaining back-ward compatibility with board-file
(non DTS) approach. Once we move to all DTS approach may be this can be
removed (but on case-by case basis).

[...]

> 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 */
>
If I'm not wrong, this is all you need in this patch
empty functions for CONFIG_MTD_NAND_ECC_BCH exposed symbols are already
handled in /include/linux/mtd/nand_bch.h

So, after this change, you can most of #ifdefs and IS_ENABLED()
and this patch should be simplified.


with regards, pekon

------------------------
Powered by BigRock.com

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

[Ezequiel Garcia]

On 07 Sep 02:40 AM, pekon wrote:
> + linux-omap, as this patch touches arch/arm/mach-omap2/gpmc.c
> 
> Hi Ezequiel,
> 
> On Sunday 07 September 2014 01:26 AM, 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).
> >
> Yup, true.
> Thanks for cleaning up OMAP2 NAND driver, but few feedbacks.
> 

First of all, this is no cleaning. The driver fails to load as a module
over here. Unless I'm missed something, ifdef seemed false when CONFIG_xxx=m.

> Just for record, current version of omap2.c NAND driver has so
> many #ifdef because:
> 
[..]

I can't think of any valid excuses for cluttering the code with ifdefs like
this. There are several ways of doing things cleaner, by grouping code
differently.

[..]
> >+	}
> >+	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;
> >  }
> 
> Actually this new function is not required.
[snip]
> If I'm not wrong, this is all you need in this patch
> empty functions for CONFIG_MTD_NAND_ECC_BCH exposed symbols are already
> handled in /include/linux/mtd/nand_bch.h
> 
> So, after this change, you can most of #ifdefs and IS_ENABLED()
> and this patch should be simplified.
> 

OK, I'll take a look. Thanks for the suggestion,
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

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

[Ezequiel Garcia]

On 07 Sep 02:40 AM, pekon wrote:
[..]
> >+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;
> >  }
> 
> Actually this new function is not required.
> We already have this check in GPMC driver when we select the ecc-scheme.
> Plz refer: File: /arch/arm/mach-omap2/gpmc.c
> 	@@ static int gpmc_probe_nand_child(...)
> 	...
> 	/* select ecc-scheme for NAND */
> 
> is_elm_enabled() is a misnomer here. It actually does not check
> for ELM DTS node, because that check is already done in GPMC driver.
> It rather checks if ELM device is correctly binded and driver
> "initialized" before NAND driver is probed.
> 
> Though ELM is not required during NAND probe because there is no error
> checking/correction required during NAND probe, but this again comes
> from the history of maintaining back-ward compatibility with board-file
> (non DTS) approach. Once we move to all DTS approach may be this can be
> removed (but on case-by case basis).
> 
> [...]
> 
> >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 */
> >
> If I'm not wrong, this is all you need in this patch
> empty functions for CONFIG_MTD_NAND_ECC_BCH exposed symbols are already
> handled in /include/linux/mtd/nand_bch.h
> 
> So, after this change, you can most of #ifdefs and IS_ENABLED()
> and this patch should be simplified.
> 

If I understand your proposal correctly you are suggesting to drop the
checks for CONFIG_MTD_NAND_ECC_BCH, CONFIG_MTD_NAND_OMAP_BCH, and the ELM
devicetree node.

However, I'd say that change is even more invasive than this one. This commit
merely replaces the current #ifdefs check with IS_ENABLED and tries to do so
in a cleaner way.

This is done on purpose, to keep the current behavior as much as possible.

In addition, if we don't check for the configs explicitly at probe time,
we would only defer the error until some later point, for instance in the call to
nand_chip->ecc.correct(). I don't think that's user-friendly.

-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

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

[pekon]

Hi Ezequiel,

On Sunday 07 September 2014 04:47 AM, Ezequiel Garcia wrote:
> On 07 Sep 02:40 AM, pekon wrote:

>> [...]
>>
>>> 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 */
>>>
>> If I'm not wrong, this is all you need in this patch
>> empty functions for CONFIG_MTD_NAND_ECC_BCH exposed symbols are already
>> handled in /include/linux/mtd/nand_bch.h
>>
>> So, after this change, you can most of #ifdefs and IS_ENABLED()
>> and this patch should be simplified.
>>
>
> If I understand your proposal correctly you are suggesting to drop the
> checks for CONFIG_MTD_NAND_ECC_BCH, CONFIG_MTD_NAND_OMAP_BCH, and the ELM
> devicetree node.
>
Absolutely ..
(1) Just the change marked above handles whether
     CONFIG_MTD_NAND_OMAP_BCH is defined or not.
(2) Same way nand_bch.c takes care whether CONFIG_MTD_NAND_ECC_BCH is
     defined or not.
(3) And gpmc.c @@gpmc_probe_nand_child(...) already checks for
     ELM DTS node.

> However, I'd say that change is even more invasive than this one. This commit
> merely replaces the current #ifdefs check with IS_ENABLED and tries to do so
> in a cleaner way.
>
I would say you can get rid of most of #ifdefs and IS_ENABLED()
in this patch itself. And testing it that should be easy, you just
need to compile with CONFIG_MTD_NAND_xxx_BCH enabled one at a time.

> This is done on purpose, to keep the current behavior as much as possible.
>
> In addition, if we don't check for the configs explicitly at probe time,
> we would only defer the error until some later point, for instance in the call to
> nand_chip->ecc.correct(). I don't think that's user-friendly.
>
As ECC-scheme is selected in GPMC driver based on DTS settings, so
any mis-match is easily handled there.
Moreover, error will occur when we change ECC-scheme on-the-fly,
which is still not supported by framework, and require many other
updates if we plan to support that in near future.
So considering ECC-scheme as static configuration is a safe assumption.

But surely you can drop new check in omap2_nand_ecc_check(), which
is already covered in @@gpmc_probe_nand_child(...)

However, I leave it to you and rogerq@ti.com (as he currently
maintains OMAP NAND driver from TI side) to decide how to go about it.


with regards, pekon

------------------------
Powered by BigRock.com

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

[Ezequiel Garcia]

On 07 Sep 03:05 PM, pekon wrote:
[..]
> As ECC-scheme is selected in GPMC driver based on DTS settings, so
> any mis-match is easily handled there.
> Moreover, error will occur when we change ECC-scheme on-the-fly,
> which is still not supported by framework, and require many other
> updates if we plan to support that in near future.
> So considering ECC-scheme as static configuration is a safe assumption.
> 
> But surely you can drop new check in omap2_nand_ecc_check(), which
> is already covered in @@gpmc_probe_nand_child(...)
> 
> However, I leave it to you and rogerq@ti.com (as he currently
> maintains OMAP NAND driver from TI side) to decide how to go about it.
> 

As far as I can see, your proposal only cover the devicetree-probed case
and it won't work for legacy boards.

Given it seems we still support legacy, I'd say your proposal would break
things.

Not to mention that I still prefer to fail at probe time if the driver cannot
deal with the selected ECC, and that's exactly what  omap2_nand_ecc_check()
does.
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

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

[Roger Quadros]

On 09/06/2014 10:56 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>

Didn't apply cleanly on 3.17-rc4. Needs a rebase?

> ---
>  drivers/mtd/nand/omap2.c          | 134 +++++++++++++++++++++-----------------
>  include/linux/platform_data/elm.h |  14 ++++
>  2 files changed, 87 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 1170389..987dd94 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;
> +		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");
> -		return -ENODEV;
> +		dev_err(&info->pdev->dev, "ELM device not found\n");
> +		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)
> +{

I like the idea of this function and it being called in probe.

It would be also be nice (maybe later) to get rid of all the ECC decision making
done in gpmc_probe_nand_child() based on presence/absence of the elm_of_node.
I guess we need to add new DT entries for "bch4sw" and "bch8sw" so that DT/platform data
can explicitly specify what it needs. I don't see how ELM works with the non-DT boot
way as there is no elm_of_node there.

Also, the elm_of_node should not be set via platform data or in gpmc.c but parsed here
directly in the NAND driver.

But changes should be done in a separate patch. Just brought them out
here for discussion.

> +	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 */
>  	nand_chip->ecc.layout	= &omap_oobinfo;
>  	ecclayout		= &omap_oobinfo;
> @@ -1826,7 +1866,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;
> @@ -1858,14 +1897,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;
> @@ -1887,21 +1920,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;
> @@ -1934,14 +1961,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;
> @@ -1953,12 +1974,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 /
> @@ -1970,14 +1992,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;
> @@ -1988,12 +2004,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 /
> @@ -2005,11 +2022,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 */
> 

Patch looks good to me. I'll give it a spin on all platforms that I have and keep you posted.

cheers,
-roger

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

[Roger Quadros]

On 09/08/2014 11:45 AM, Roger Quadros wrote:
> On 09/06/2014 10:56 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>
> 
> Didn't apply cleanly on 3.17-rc4. Needs a rebase?
> 
>> ---
>>  drivers/mtd/nand/omap2.c          | 134 +++++++++++++++++++++-----------------
>>  include/linux/platform_data/elm.h |  14 ++++
>>  2 files changed, 87 insertions(+), 61 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
>> index 1170389..987dd94 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;
>> +		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");
>> -		return -ENODEV;
>> +		dev_err(&info->pdev->dev, "ELM device not found\n");
>> +		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)
>> +{
> 
> I like the idea of this function and it being called in probe.
> 
> It would be also be nice (maybe later) to get rid of all the ECC decision making
> done in gpmc_probe_nand_child() based on presence/absence of the elm_of_node.
> I guess we need to add new DT entries for "bch4sw" and "bch8sw" so that DT/platform data
> can explicitly specify what it needs. I don't see how ELM works with the non-DT boot
> way as there is no elm_of_node there.
> 
> Also, the elm_of_node should not be set via platform data or in gpmc.c but parsed here
> directly in the NAND driver.
> 
> But changes should be done in a separate patch. Just brought them out
> here for discussion.
> 
>> +	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 */
>>  	nand_chip->ecc.layout	= &omap_oobinfo;
>>  	ecclayout		= &omap_oobinfo;
>> @@ -1826,7 +1866,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;
>> @@ -1858,14 +1897,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;
>> @@ -1887,21 +1920,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;
>> @@ -1934,14 +1961,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;
>> @@ -1953,12 +1974,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 /
>> @@ -1970,14 +1992,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;
>> @@ -1988,12 +2004,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 /
>> @@ -2005,11 +2022,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)

I still get the following error if I set CONFIG_MTD_NAND_OMAP2 to y and
CONFIG_MTD_NAND_OMAP_BCH to m.

CONFIG_MTD_NAND_OMAP_BCH is used to select the ELM driver and it must be limited to
be built-in if CONFIG_MTD_NAND_OMAP2 is built-in.

Maybe it should be a sub option of CONFIG_MTD_NAND_OMAP2.
IMHO the elm.c file must be moved from mtd/devices to mtd/nand and renamed to omap_elm.c

drivers/built-in.o: In function `omap_nand_probe':
/work/linux-2.6/drivers/mtd/nand/omap2.c:2010: undefined reference to `elm_config'
/work/linux-2.6/drivers/mtd/nand/omap2.c:1980: undefined reference to `elm_config'
/work/linux-2.6/drivers/mtd/nand/omap2.c:1927: undefined reference to `elm_config'
drivers/built-in.o: In function `omap_elm_correct_data':
/work/linux-2.6/drivers/mtd/nand/omap2.c:1444: undefined reference to `elm_decode_bch_error_page'
make: *** [vmlinux] Error 1

>>  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 */
>>
> 

cheers,
-roger

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

[Ezequiel Garcia]

On 08 Sep 01:53 PM, Roger Quadros wrote:
> On 09/08/2014 11:45 AM, Roger Quadros wrote:
> > On 09/06/2014 10:56 PM, Ezequiel Garcia wrote:
[..]
> >> 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)
> 
> I still get the following error if I set CONFIG_MTD_NAND_OMAP2 to y and
> CONFIG_MTD_NAND_OMAP_BCH to m.
> 
> CONFIG_MTD_NAND_OMAP_BCH is used to select the ELM driver and it must be limited to
> be built-in if CONFIG_MTD_NAND_OMAP2 is built-in.
>

Hm, yup. Any ideas on how to accomplish that? My Kconfig-foo is not strong enough :(
 
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

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

[Roger Quadros]

On 09/10/2014 03:48 PM, Ezequiel Garcia wrote:
> On 08 Sep 01:53 PM, Roger Quadros wrote:
>> On 09/08/2014 11:45 AM, Roger Quadros wrote:
>>> On 09/06/2014 10:56 PM, Ezequiel Garcia wrote:
> [..]
>>>> 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)
>>
>> I still get the following error if I set CONFIG_MTD_NAND_OMAP2 to y and
>> CONFIG_MTD_NAND_OMAP_BCH to m.
>>
>> CONFIG_MTD_NAND_OMAP_BCH is used to select the ELM driver and it must be limited to
>> be built-in if CONFIG_MTD_NAND_OMAP2 is built-in.
>>
> 
> Hm, yup. Any ideas on how to accomplish that? My Kconfig-foo is not strong enough :(
>  
> 
Mine neither ;). I'm unaware of any other method than making CONFIG_MTD_NAND_OMAP_BCH to bool.

cheers,
-roger

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

[Brian Norris]

On Wed, Sep 10, 2014 at 04:05:56PM +0300, Roger Quadros wrote:
> On 09/10/2014 03:48 PM, Ezequiel Garcia wrote:
> > On 08 Sep 01:53 PM, Roger Quadros wrote:
> >> On 09/08/2014 11:45 AM, Roger Quadros wrote:
> >>> On 09/06/2014 10:56 PM, Ezequiel Garcia wrote:
> >> I still get the following error if I set CONFIG_MTD_NAND_OMAP2 to y and
> >> CONFIG_MTD_NAND_OMAP_BCH to m.
> >>
> >> CONFIG_MTD_NAND_OMAP_BCH is used to select the ELM driver and it must be limited to
> >> be built-in if CONFIG_MTD_NAND_OMAP2 is built-in.
> >>
> > 
> > Hm, yup. Any ideas on how to accomplish that? My Kconfig-foo is not strong enough :(
>
> Mine neither ;). I'm unaware of any other method than making CONFIG_MTD_NAND_OMAP_BCH to bool.

That could be part of the solution. Does the following patch work? It
now forces elm and omap to be built the same -- either both as modules
or both built-in -- but it solves the rest of the problem I think.

(This also has the side effect of causing transition pains for any
.config file that had CONFIG_MTD_NAND_OMAP_BCH=m, since this becomes
boolean now.)

diff --git a/drivers/mtd/devices/Makefile b/drivers/mtd/devices/Makefile
index c68868f60588..4dc8990898dc 100644
--- a/drivers/mtd/devices/Makefile
+++ b/drivers/mtd/devices/Makefile
@@ -12,7 +12,7 @@ obj-$(CONFIG_MTD_LART)		+= lart.o
 obj-$(CONFIG_MTD_BLOCK2MTD)	+= block2mtd.o
 obj-$(CONFIG_MTD_DATAFLASH)	+= mtd_dataflash.o
 obj-$(CONFIG_MTD_M25P80)	+= m25p80.o
-obj-$(CONFIG_MTD_NAND_OMAP_BCH)	+= elm.o
+obj-$(CONFIG_MTD_NAND_OMAP_BCH_BUILD) += elm.o
 obj-$(CONFIG_MTD_SPEAR_SMI)	+= spear_smi.o
 obj-$(CONFIG_MTD_SST25L)	+= sst25l.o
 obj-$(CONFIG_MTD_BCM47XXSFLASH)	+= bcm47xxsflash.o
diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index f1cf503517fd..6390b2b58edb 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -96,9 +96,8 @@ config MTD_NAND_OMAP2
 
 config MTD_NAND_OMAP_BCH
 	depends on MTD_NAND_OMAP2
-	tristate "Support hardware based BCH error correction"
+	bool "Support hardware based BCH error correction"
 	default n
-	select BCH
 	help
 	  This config enables the ELM hardware engine, which can be used to
 	  locate and correct errors when using BCH ECC scheme. This offloads
@@ -106,6 +105,12 @@ config MTD_NAND_OMAP_BCH
 	  legacy OMAP families like OMAP2xxx, OMAP3xxx do not have ELM engine
 	  so they should not enable this config symbol.
 
+config MTD_NAND_OMAP_BCH_BUILD
+	tristate
+	depends on MTD_NAND_OMAP_BCH
+	default MTD_NAND_OMAP2
+	select BCH
+
 config MTD_NAND_IDS
 	tristate
 

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

[Ezequiel Garcia]

On 17 September 2014 09:33, Brian Norris <computersforpeace@gmail.com> wrote:
> On Wed, Sep 10, 2014 at 04:05:56PM +0300, Roger Quadros wrote:
>> On 09/10/2014 03:48 PM, Ezequiel Garcia wrote:
>> > On 08 Sep 01:53 PM, Roger Quadros wrote:
>> >> On 09/08/2014 11:45 AM, Roger Quadros wrote:
>> >>> On 09/06/2014 10:56 PM, Ezequiel Garcia wrote:
>> >> I still get the following error if I set CONFIG_MTD_NAND_OMAP2 to y and
>> >> CONFIG_MTD_NAND_OMAP_BCH to m.
>> >>
>> >> CONFIG_MTD_NAND_OMAP_BCH is used to select the ELM driver and it must be limited to
>> >> be built-in if CONFIG_MTD_NAND_OMAP2 is built-in.
>> >>
>> >
>> > Hm, yup. Any ideas on how to accomplish that? My Kconfig-foo is not strong enough :(
>>
>> Mine neither ;). I'm unaware of any other method than making CONFIG_MTD_NAND_OMAP_BCH to bool.
>
> That could be part of the solution. Does the following patch work? It
> now forces elm and omap to be built the same -- either both as modules
> or both built-in -- but it solves the rest of the problem I think.
>
> (This also has the side effect of causing transition pains for any
> .config file that had CONFIG_MTD_NAND_OMAP_BCH=m, since this becomes
> boolean now.)
>

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

I used my time-machine to go back in time and submitted the patch for you :-)
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

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

[Brian Norris]

On Wed, Sep 17, 2014 at 10:54:03AM +0100, Ezequiel Garcia wrote:
> On 17 September 2014 09:33, Brian Norris <computersforpeace@gmail.com> wrote:
> > On Wed, Sep 10, 2014 at 04:05:56PM +0300, Roger Quadros wrote:
> >> On 09/10/2014 03:48 PM, Ezequiel Garcia wrote:
> >> > On 08 Sep 01:53 PM, Roger Quadros wrote:
> >> >> On 09/08/2014 11:45 AM, Roger Quadros wrote:
> >> >>> On 09/06/2014 10:56 PM, Ezequiel Garcia wrote:
> >> >> I still get the following error if I set CONFIG_MTD_NAND_OMAP2 to y and
> >> >> CONFIG_MTD_NAND_OMAP_BCH to m.
> >> >>
> >> >> CONFIG_MTD_NAND_OMAP_BCH is used to select the ELM driver and it must be limited to
> >> >> be built-in if CONFIG_MTD_NAND_OMAP2 is built-in.
> >> >>
> >> >
> >> > Hm, yup. Any ideas on how to accomplish that? My Kconfig-foo is not strong enough :(
> >>
> >> Mine neither ;). I'm unaware of any other method than making CONFIG_MTD_NAND_OMAP_BCH to bool.
> >
> > That could be part of the solution. Does the following patch work? It
> > now forces elm and omap to be built the same -- either both as modules
> > or both built-in -- but it solves the rest of the problem I think.
> >
> > (This also has the side effect of causing transition pains for any
> > .config file that had CONFIG_MTD_NAND_OMAP_BCH=m, since this becomes
> > boolean now.)
> >
> 
> http://patchwork.ozlabs.org/patch/388249/
> 
> I used my time-machine to go back in time and submitted the patch for you :-)

I'm glad you prioritized MTD over all the other cool things you could do
with your time machine. But I guess, you really don't need to prioritize
much when you have infinite time, do you?

</joke>

I can get lost when I only pay 50% attention for a few weeks, then try
to sort through the accumulated pile of email.

Brian

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

[pekon]

On Wednesday 10 September 2014 06:18 PM, Ezequiel Garcia wrote:
> On 08 Sep 01:53 PM, Roger Quadros wrote:
>> On 09/08/2014 11:45 AM, Roger Quadros wrote:
>>> On 09/06/2014 10:56 PM, Ezequiel Garcia wrote:
> [..]
>>>> 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)
>>
>> I still get the following error if I set CONFIG_MTD_NAND_OMAP2 to y and
>> CONFIG_MTD_NAND_OMAP_BCH to m.
>>
>> CONFIG_MTD_NAND_OMAP_BCH is used to select the ELM driver and it must be limited to
>> be built-in if CONFIG_MTD_NAND_OMAP2 is built-in.
>>
>
> Hm, yup. Any ideas on how to accomplish that? My Kconfig-foo is not strong enough :(
>
>
Does following work ?
config MTD_NAND_OMAP_BCH
	depends on MTD_NAND_OMAP2
	tristate "<message>" if (MTD_NAND_OMAP2 && m)
	bool	 "<message>" if (MTD_NAND_OMAP2 && y)
	default n
	...

Sorry, I don't have tools to check myself. If not following should help:
$KERNEL/Documentation/kbuild/kconfig-language.txt


with regards, pekon

------------------------
Powered by BigRock.com

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

[Ezequiel Garcia]

On 08 Sep 11:45 AM, Roger Quadros wrote:
> On 09/06/2014 10:56 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>
> 
> Didn't apply cleanly on 3.17-rc4. Needs a rebase?
> 

Just sent a new version rebased on v3.17-rc4.

Thanks a lot for the test!
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

[PATCH 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 | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 987dd94..8521f0d 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;
 				}
 			}
@@ -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;
 	}
@@ -1892,10 +1894,8 @@ static int omap_nand_probe(struct platform_device *pdev)
 							nand_chip->ecc.size,
 							nand_chip->ecc.bytes,
 							&nand_chip->ecc.layout);
-		if (!nand_chip->ecc.priv) {
-			pr_err("nand: error: unable to use s/w BCH library\n");
+		if (!nand_chip->ecc.priv)
 			err = -EINVAL;
-		}
 		break;
 
 	case OMAP_ECC_BCH4_CODE_HW:
@@ -1956,7 +1956,6 @@ static int omap_nand_probe(struct platform_device *pdev)
 							nand_chip->ecc.bytes,
 							&nand_chip->ecc.layout);
 		if (!nand_chip->ecc.priv) {
-			pr_err("nand: error: unable to use s/w BCH library\n");
 			err = -EINVAL;
 			goto return_error;
 		}
@@ -2023,7 +2022,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;
 	}
@@ -2032,8 +2031,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.0.1

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

[Roger Quadros]

Hi Ezequiel,

On 09/06/2014 10:56 PM, Ezequiel Garcia wrote:
> Hi Brian, Roger:
> 
> 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.
> 
> Roger, do you think you can test patches 1 and 2 with different ECCs
> and configurations? It's an invasive patch and I don't want to see more
> regressions with this driver.

Yes. I will run them through all the boards that I have and let you know in
a day or two.

> 
> And speaking of modules, the driver loads as "modprobe omap2". And it's not
> the only one with a clumsy name: "modprobe elm". I guess we cannot fix it now,
> but it would be great to be more careful with driver naming in the future.

Why can't we fix it now? It seems nobody ever used it as a module before so now
is the right time to fix. Unless Tony has any objections.

cheers,
-roger

> 
> Ezequiel Garcia (3):
>   nand: omap2: Add support for a flash-based bad block table
>   nand: omap2: Refactor the code to remove horrible ifdefs
>   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, 108 insertions(+), 75 deletions(-)
> 

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

[Ezequiel Garcia]

On 08 Sep 11:30 AM, Roger Quadros wrote:
[..]
> > 
> > And speaking of modules, the driver loads as "modprobe omap2". And it's not
> > the only one with a clumsy name: "modprobe elm". I guess we cannot fix it now,
> > but it would be great to be more careful with driver naming in the future.
> 
> Why can't we fix it now? It seems nobody ever used it as a module before so now
> is the right time to fix. Unless Tony has any objections.
> 

That would be great. "omap2-nand" or something like that would be optimal.

Regarding the ELM driver, I agree with your suggestion in the other thread,
we could just move it into drivers/mtd/nand and rework it to not be a module
but just a pack of helpers for the NAND driver.

Do you think that would work?
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

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

[Roger Quadros]

On 09/08/2014 02:31 PM, Ezequiel Garcia wrote:
> On 08 Sep 11:30 AM, Roger Quadros wrote:
> [..]
>>>
>>> And speaking of modules, the driver loads as "modprobe omap2". And it's not
>>> the only one with a clumsy name: "modprobe elm". I guess we cannot fix it now,
>>> but it would be great to be more careful with driver naming in the future.
>>
>> Why can't we fix it now? It seems nobody ever used it as a module before so now
>> is the right time to fix. Unless Tony has any objections.
>>
> 
> That would be great. "omap2-nand" or something like that would be optimal.

Other drivers seem to use and underscore e.g. pxa3xx_nand.c, orion_nand.c

so "omap2_nand" is fine by me.

> 
> Regarding the ELM driver, I agree with your suggestion in the other thread,
> we could just move it into drivers/mtd/nand and rework it to not be a module
> but just a pack of helpers for the NAND driver.
> 
> Do you think that would work?
> 

The ELM is in fact a separate IP block on the SoC with its own Clocking, reset mechanism
and memory resource. Clocking and reset is handled by OMAP hwmod framework, so it might not
be that easy to get rid of the ELM device driver and make it just a library.

For now just moving it to mtd/nand and naming it omap_elm (or omap4_elm since it was added in
OMAP4) is good enough IMO.

cheers,
-roger