Re: [PATCH v6 2/4] mtd:nand:omap2: clean-up BCHx_HW and BCHx_SW ECC configurations in device_probe
BTW, I'll elaborate on a few things that are hidden in the noise here.
On Thu, Sep 12, 2013 at 05:20:17PM +0530, Pekon Gupta wrote:
> OMAP NAND driver support multiple ECC scheme, which can used in following
> different flavours, depending on in-build Hardware engines supported by SoC.
...
> This patch
> - removes OMAP_ECC_HAMMING_CODE_DEFAULT and OMAP_ECC_HAMMING_CODE_HW_ROMCODE
> - separates the configurations for other ECC schemes
> - fixes dependency issues based on Kconfig options
> - updates ELM device detection in is_elm_present()
> - cleans redundant code
>
> Signed-off-by: Pekon Gupta
> ---
> drivers/mtd/nand/omap2.c | 530
> +--
> include/linux/platform_data/mtd-nand-omap2.h | 7 +-
> 2 files changed, 260 insertions(+), 277 deletions(-)
>
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 4ecf0e5..420078f 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
...
> @@ -1846,20 +1720,20 @@ static int omap_nand_probe(struct platform_device
> *pdev)
> spin_lock_init(&info->controller.lock);
> init_waitqueue_head(&info->controller.wq);
>
> - info->pdev = pdev;
> + mtd = &info->mtd;
> + mtd->name = dev_name(&pdev->dev);
> + mtd->owner = THIS_MODULE;
> + mtd->priv = &info->nand;
> + chip= mtd->priv;
>
> + info->pdev = pdev;
> info->gpmc_cs = pdata->cs;
> info->reg = pdata->reg;
> + info->bch = NULL;
>
> - info->mtd.priv = &info->nand;
> - info->mtd.name = dev_name(&pdev->dev);
> - info->mtd.owner = THIS_MODULE;
> -
> - info->nand.options = pdata->devsize;
> + info->nand.options = NAND_BUSWIDTH_AUTO;
You're changing the buswidth detection significantly. You don't even
mention that in the commit description. This should be a separate patch.
> info->nand.options |= NAND_SKIP_BBTSCAN;
> -#ifdef CONFIG_MTD_NAND_OMAP_BCH
> info->of_node = pdata->of_node;
> -#endif
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> if (res == NULL) {
> @@ -1903,6 +1777,30 @@ static int omap_nand_probe(struct platform_device
> *pdev)
> info->nand.chip_delay = 50;
> }
>
> + /* scan NAND device conncted to controller */
> + if (nand_scan_ident(mtd, 1, NULL)) {
> + err = -ENXIO;
> + goto out_release_mem_region;
> + }
> + pr_info("%s: detected %s NAND flash\n", DRIVER_NAME,
> + (info->nand.options & NAND_BUSWIDTH_16) ? "x16" : "x8");
Do you really need to print this unconditionally? nand_base already has
some device info printed out (not the buswidth), so if it's really
needed, it should go there. But we really don't need to print out all
this IMO.
Perhaps move the print into the 'if' block, so that you only alert the
user about the buswidth if it doesn't match.
> + if ((info->nand.options & NAND_BUSWIDTH_16) !=
> + (pdata->devsize & NAND_BUSWIDTH_16)) {
> + pr_err("%s: but incorrectly configured as %s", DRIVER_NAME,
> + (pdata->devsize & NAND_BUSWIDTH_16) ? "x16" : "x8");
> + err = -EINVAL;
> + goto out_release_mem_region;
> + }
> +
> + /* check for small page devices */
> + if ((mtd->oobsize < 64) &&
> + (pdata->ecc_opt != OMAP_ECC_HAMMING_CODE_HW)) {
> + pr_err("small page devices are not supported\n");
> + err = -EINVAL;
> + goto out_release_mem_region;
> + }
> +
> + /* populate read & write API based on xfer_type selected */
> switch (pdata->xfer_type) {
> case NAND_OMAP_PREFETCH_POLLED:
> info->nand.read_buf = omap_read_buf_pref;
> @@ -1992,64 +1890,152 @@ static int omap_nand_probe(struct platform_device
> *pdev)
> goto out_release_mem_region;
> }
>
> - /* select the ecc type */
> - if (pdata->ecc_opt == OMAP_ECC_HAMMING_CODE_DEFAULT)
> - info->nand.ecc.mode = NAND_ECC_SOFT;
> - else if ((pdata->ecc_opt == OMAP_ECC_HAMMING_CODE_HW) ||
> - (pdata->ecc_opt == OMAP_ECC_HAMMING_CODE_HW_ROMCODE)) {
> + /* populate MTD interface based on ECC scheme */
> + chip->ecclayout = &omap_oobinfo;
This field was recently removed in l2-mtd.git. It was redundant with
chip->ecc.layout.
> + chip->ecc.layout= &omap_oobinfo;
> + ecclayout = &omap_oobinfo;
> + switch (pdata->ecc_opt) {
> + case OMAP_ECC_HAMMING_CODE_HW:
> + pr_info("nand: using OMAP_ECC_HAMMING_CODE_HW\n");
> + info->nand.ecc.mode = NAND_ECC_HW;
> info->nand.ecc.bytes= 3;
> info->nand.ecc.size = 512;
> info->nand.ecc.str
Re: [PATCH v6 2/4] mtd:nand:omap2: clean-up BCHx_HW and BCHx_SW ECC configurations in device_probe
Hi Pekon, On Thu, Sep 12, 2013 at 05:20:17PM +0530, Pekon Gupta wrote: > OMAP NAND driver support multiple ECC scheme, which can used in following > different flavours, depending on in-build Hardware engines supported by SoC. > > +---+---+---+ > | ECC scheme|ECC calculation|Error detection| > +---+---+---+ > |OMAP_ECC_HAMMING_CODE_HW |H/W (GPMC) |S/W| > +---+---+---+ > |OMAP_ECC_BCH8_CODE_HW_DETECTION_SW |H/W (GPMC) |S/W| > |(requires CONFIG_MTD_NAND_ECC_BCH) | | | > +---+---+---+ > |OMAP_ECC_BCH8_CODE_HW |H/W (GPMC) |H/W (ELM) | > |(requires CONFIG_MTD_NAND_OMAP_BCH && | | | > | ti,elm-id in DT) | | | > +---+---+---+ > To optimize footprint of omap2-nand driver, selection of some ECC schemes > also require enabling following Kconfigs, in addition to setting appropriate > DT bindings > - Kconfig:CONFIG_MTD_NAND_ECC_BCHenables S/W based BCH ECC algorithm > - Kconfig:CONFIG_MTD_NAND_OMAP_BCH enables H/W based BCH ECC algorithm > > This patch ... does too many unrelated things in a single patch. I am not comfortable taking large amounts of refactoring mixed in with Kconfig and #ifdef changes. Can you please separate the steps you list below into multiple patches and describe each one? I think you are doing many trivial things, but it's difficult to separate the noise out from the substantial changes. Or, you can wait even longer until I have more time to kill :) > - removes OMAP_ECC_HAMMING_CODE_DEFAULT and OMAP_ECC_HAMMING_CODE_HW_ROMCODE > - separates the configurations for other ECC schemes > - fixes dependency issues based on Kconfig options > - updates ELM device detection in is_elm_present() > - cleans redundant code Brian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 2/4] mtd:nand:omap2: clean-up BCHx_HW and BCHx_SW ECC configurations in device_probe
OMAP NAND driver support multiple ECC scheme, which can used in following
different flavours, depending on in-build Hardware engines supported by SoC.
+---+---+---+
| ECC scheme|ECC calculation|Error detection|
+---+---+---+
|OMAP_ECC_HAMMING_CODE_HW |H/W (GPMC) |S/W|
+---+---+---+
|OMAP_ECC_BCH8_CODE_HW_DETECTION_SW |H/W (GPMC) |S/W|
|(requires CONFIG_MTD_NAND_ECC_BCH) | | |
+---+---+---+
|OMAP_ECC_BCH8_CODE_HW |H/W (GPMC) |H/W (ELM) |
|(requires CONFIG_MTD_NAND_OMAP_BCH && | | |
| ti,elm-id in DT) | | |
+---+---+---+
To optimize footprint of omap2-nand driver, selection of some ECC schemes
also require enabling following Kconfigs, in addition to setting appropriate
DT bindings
- Kconfig:CONFIG_MTD_NAND_ECC_BCHenables S/W based BCH ECC algorithm
- Kconfig:CONFIG_MTD_NAND_OMAP_BCH enables H/W based BCH ECC algorithm
This patch
- removes OMAP_ECC_HAMMING_CODE_DEFAULT and OMAP_ECC_HAMMING_CODE_HW_ROMCODE
- separates the configurations for other ECC schemes
- fixes dependency issues based on Kconfig options
- updates ELM device detection in is_elm_present()
- cleans redundant code
Signed-off-by: Pekon Gupta
---
drivers/mtd/nand/omap2.c | 530 +--
include/linux/platform_data/mtd-nand-omap2.h | 7 +-
2 files changed, 260 insertions(+), 277 deletions(-)
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 4ecf0e5..420078f 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -25,8 +25,10 @@
#include
#include
-#ifdef CONFIG_MTD_NAND_OMAP_BCH
+#ifdef CONFIG_MTD_NAND_ECC_BCH
#include
+#endif
+#ifdef CONFIG_MTD_NAND_OMAP_BCH
#include
#endif
@@ -141,6 +143,9 @@
#define BCH_ECC_SIZE0 0x0 /* ecc_size0 = 0, no oob protection */
#define BCH_ECC_SIZE1 0x20/* ecc_size1 = 32 */
+#define BADBLOCK_MARKER_LENGTH 0x2
+#define OMAP_ECC_BCH8_POLYNOMIAL 0x201b
+
#ifdef CONFIG_MTD_NAND_OMAP_BCH
static u_char bch8_vector[] = {0xf3, 0xdb, 0x14, 0x16, 0x8b, 0xd2, 0xbe, 0xcc,
0xac, 0x6b, 0xff, 0x99, 0x7b};
@@ -182,14 +187,11 @@ struct omap_nand_info {
u_char *buf;
int buf_len;
struct gpmc_nand_regs reg;
-
-#ifdef CONFIG_MTD_NAND_OMAP_BCH
- struct bch_control *bch;
- struct nand_ecclayout ecclayout;
+ /* fields specific for BCHx_HW ECC scheme */
+ struct bch_control *bch;
boolis_elm_used;
struct device *elm_dev;
struct device_node *of_node;
-#endif
};
/**
@@ -1058,8 +1060,6 @@ static int omap_dev_ready(struct mtd_info *mtd)
}
}
-#ifdef CONFIG_MTD_NAND_OMAP_BCH
-
/**
* omap3_enable_hwecc_bch - Program OMAP3 GPMC to perform BCH ECC correction
* @mtd: MTD device structure
@@ -1141,6 +1141,7 @@ static void omap3_enable_hwecc_bch(struct mtd_info *mtd,
int mode)
writel(ECCCLEAR | ECC1, info->reg.gpmc_ecc_control);
}
+#ifdef CONFIG_MTD_NAND_ECC_BCH
/**
* omap3_calculate_ecc_bch4 - Generate 7 bytes of ECC bytes
* @mtd: MTD device structure
@@ -1227,6 +1228,62 @@ static int omap3_calculate_ecc_bch8(struct mtd_info
*mtd, const u_char *dat,
}
/**
+ * omap3_correct_data_bch - Decode received data and correct errors
+ * @mtd: MTD device structure
+ * @data: page data
+ * @read_ecc: ecc read from nand flash
+ * @calc_ecc: ecc read from HW ECC registers
+ */
+static int omap3_correct_data_bch(struct mtd_info *mtd, u_char *data,
+ u_char *read_ecc, u_char *calc_ecc)
+{
+ int i, count;
+ /* cannot correct more than 8 errors */
+ unsigned int errloc[8];
+ struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
+ mtd);
+
+ count = decode_bch(info->bch, NULL, 512, read_ecc, calc_ecc, NULL,
+ errloc);
+ if (count > 0) {
+ /* correct errors */
+ for (i = 0; i < count; i++) {
+ /* correct data only, not ecc bytes */
+ if (errloc[i] < 8*512)
+ data[errloc[i]/8] ^= 1 << (errloc[i] & 7);
+ pr_debug("corrected bitflip %u\n", errloc[i]);
+ }
+ } else if (count < 0) {
+ pr_err("ecc unrecoverable error\n");
+
