2015-04-22 08:18:20

by Tomas Hlavacek

[permalink] [raw]
Subject: [RFC] mtd: fsl_elbc_nand Add ECC mode selection in DT

Add device tree parameters to turn off the HW ECC and force own ECC mode
and ECC parameters. New entries are: nand-ecc-mode, nand-ecc-step-size
and nand-ecc-strength.

Add RNDOUT operation which is required for SOFT and SOFT_BCH modes.

Do not set write_subpage function pointer from the driver when it initializes
in SOFT and SOFT_BCH modes.

Signed-off-by: Tomas Hlavacek <[email protected]>
---
drivers/mtd/nand/fsl_elbc_nand.c | 47 +++++++++++++++++++++++++++++++++++++---
1 file changed, 44 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c
index 04b22fd..76ab2e2 100644
--- a/drivers/mtd/nand/fsl_elbc_nand.c
+++ b/drivers/mtd/nand/fsl_elbc_nand.c
@@ -335,6 +335,14 @@ static void fsl_elbc_cmdfunc(struct mtd_info *mtd, unsigned int command,
fsl_elbc_run_command(mtd);
return;

+ case NAND_CMD_RNDOUT:
+ dev_vdbg(priv->dev,
+ "fsl_elbc_cmdfunc: NAND_CMD_RNDOUT, column: 0x%x.\n",
+ column);
+
+ elbc_fcm_ctrl->index = column;
+ return;
+
/* READOOB reads only the OOB because no ECC is performed. */
case NAND_CMD_READOOB:
dev_vdbg(priv->dev,
@@ -656,6 +664,10 @@ static int fsl_elbc_chip_init_tail(struct mtd_info *mtd)
chip->ecc.steps);
dev_dbg(priv->dev, "fsl_elbc_init: nand->ecc.bytes = %d\n",
chip->ecc.bytes);
+ dev_dbg(priv->dev, "fsl_elbc_init: nand->ecc.size = %d\n",
+ chip->ecc.size);
+ dev_dbg(priv->dev, "fsl_elbc_init: nand->ecc.strength = %d\n",
+ chip->ecc.strength);
dev_dbg(priv->dev, "fsl_elbc_init: nand->ecc.total = %d\n",
chip->ecc.total);
dev_dbg(priv->dev, "fsl_elbc_init: nand->ecc.layout = %p\n",
@@ -677,8 +689,8 @@ static int fsl_elbc_chip_init_tail(struct mtd_info *mtd)
priv->page_size = 1;
setbits32(&lbc->bank[priv->bank].or, OR_FCM_PGS);
/* adjust ecc setup if needed */
- if ((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) ==
- BR_DECC_CHK_GEN) {
+ if (((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) ==
+ BR_DECC_CHK_GEN) && (chip->ecc.mode == NAND_ECC_HW)) {
chip->ecc.size = 512;
chip->ecc.layout = (priv->fmr & FMR_ECCM) ?
&fsl_elbc_oob_lp_eccm1 :
@@ -742,6 +754,8 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)
struct fsl_lbc_regs __iomem *lbc = ctrl->regs;
struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = ctrl->nand;
struct nand_chip *chip = &priv->chip;
+ struct device_node *node = priv->dev->of_node;
+ const char *ecc_mode;

dev_dbg(priv->dev, "eLBC Set Information for bank %d\n", priv->bank);

@@ -774,11 +788,38 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)

chip->ecc.read_page = fsl_elbc_read_page;
chip->ecc.write_page = fsl_elbc_write_page;
- chip->ecc.write_subpage = fsl_elbc_write_subpage;
+
+ /* Override default HW ECC according to settings in DT */
+ if (!of_property_read_string(node, "nand-ecc-mode", &ecc_mode)) {
+ if (!strncmp("none", ecc_mode, 4)) {
+ chip->ecc.mode = NAND_ECC_NONE;
+ return 0;
+ }
+
+ if (!strncmp("soft_bch", ecc_mode, 8)) {
+ chip->ecc.mode = NAND_ECC_SOFT_BCH;
+ chip->ecc.size = 512;
+ chip->ecc.strength = 4;
+
+ of_property_read_u32(node, "nand-ecc-step-size",
+ &chip->ecc.size);
+ of_property_read_u32(node, "nand-ecc-strength",
+ &chip->ecc.strength);
+ chip->ecc.bytes = ((fls(1+8*chip->ecc.size)*
+ chip->ecc.strength)/8);
+ return 0;
+ }
+
+ if (!strncmp("soft", ecc_mode, 4)) {
+ chip->ecc.mode = NAND_ECC_SOFT;
+ return 0;
+ }
+ }

/* If CS Base Register selects full hardware ECC then use it */
if ((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) ==
BR_DECC_CHK_GEN) {
+ chip->ecc.write_subpage = fsl_elbc_write_subpage;
chip->ecc.mode = NAND_ECC_HW;
/* put in small page settings and adjust later if needed */
chip->ecc.layout = (priv->fmr & FMR_ECCM) ?
--
2.1.4


2015-05-07 23:15:37

by Brian Norris

[permalink] [raw]
Subject: Re: [RFC] mtd: fsl_elbc_nand Add ECC mode selection in DT

On Wed, Apr 22, 2015 at 10:18:10AM +0200, Tomas Hlavacek wrote:
> Add device tree parameters to turn off the HW ECC and force own ECC mode
> and ECC parameters. New entries are: nand-ecc-mode, nand-ecc-step-size
> and nand-ecc-strength.
>
> Add RNDOUT operation which is required for SOFT and SOFT_BCH modes.
>
> Do not set write_subpage function pointer from the driver when it initializes
> in SOFT and SOFT_BCH modes.
>
> Signed-off-by: Tomas Hlavacek <[email protected]>
> ---
> drivers/mtd/nand/fsl_elbc_nand.c | 47 +++++++++++++++++++++++++++++++++++++---
> 1 file changed, 44 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c
> index 04b22fd..76ab2e2 100644
> --- a/drivers/mtd/nand/fsl_elbc_nand.c
> +++ b/drivers/mtd/nand/fsl_elbc_nand.c
> @@ -335,6 +335,14 @@ static void fsl_elbc_cmdfunc(struct mtd_info *mtd, unsigned int command,
> fsl_elbc_run_command(mtd);
> return;
>
> + case NAND_CMD_RNDOUT:
> + dev_vdbg(priv->dev,
> + "fsl_elbc_cmdfunc: NAND_CMD_RNDOUT, column: 0x%x.\n",
> + column);
> +
> + elbc_fcm_ctrl->index = column;
> + return;
> +
> /* READOOB reads only the OOB because no ECC is performed. */
> case NAND_CMD_READOOB:
> dev_vdbg(priv->dev,
> @@ -656,6 +664,10 @@ static int fsl_elbc_chip_init_tail(struct mtd_info *mtd)
> chip->ecc.steps);
> dev_dbg(priv->dev, "fsl_elbc_init: nand->ecc.bytes = %d\n",
> chip->ecc.bytes);
> + dev_dbg(priv->dev, "fsl_elbc_init: nand->ecc.size = %d\n",
> + chip->ecc.size);
> + dev_dbg(priv->dev, "fsl_elbc_init: nand->ecc.strength = %d\n",
> + chip->ecc.strength);
> dev_dbg(priv->dev, "fsl_elbc_init: nand->ecc.total = %d\n",
> chip->ecc.total);
> dev_dbg(priv->dev, "fsl_elbc_init: nand->ecc.layout = %p\n",
> @@ -677,8 +689,8 @@ static int fsl_elbc_chip_init_tail(struct mtd_info *mtd)
> priv->page_size = 1;
> setbits32(&lbc->bank[priv->bank].or, OR_FCM_PGS);
> /* adjust ecc setup if needed */
> - if ((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) ==
> - BR_DECC_CHK_GEN) {
> + if (((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) ==
> + BR_DECC_CHK_GEN) && (chip->ecc.mode == NAND_ECC_HW)) {
> chip->ecc.size = 512;
> chip->ecc.layout = (priv->fmr & FMR_ECCM) ?
> &fsl_elbc_oob_lp_eccm1 :
> @@ -742,6 +754,8 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)
> struct fsl_lbc_regs __iomem *lbc = ctrl->regs;
> struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = ctrl->nand;
> struct nand_chip *chip = &priv->chip;
> + struct device_node *node = priv->dev->of_node;
> + const char *ecc_mode;
>
> dev_dbg(priv->dev, "eLBC Set Information for bank %d\n", priv->bank);
>
> @@ -774,11 +788,38 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)
>
> chip->ecc.read_page = fsl_elbc_read_page;
> chip->ecc.write_page = fsl_elbc_write_page;
> - chip->ecc.write_subpage = fsl_elbc_write_subpage;
> +
> + /* Override default HW ECC according to settings in DT */
> + if (!of_property_read_string(node, "nand-ecc-mode", &ecc_mode)) {
> + if (!strncmp("none", ecc_mode, 4)) {
> + chip->ecc.mode = NAND_ECC_NONE;
> + return 0;
> + }
> +
> + if (!strncmp("soft_bch", ecc_mode, 8)) {
> + chip->ecc.mode = NAND_ECC_SOFT_BCH;
> + chip->ecc.size = 512;
> + chip->ecc.strength = 4;
> +
> + of_property_read_u32(node, "nand-ecc-step-size",
> + &chip->ecc.size);
> + of_property_read_u32(node, "nand-ecc-strength",
> + &chip->ecc.strength);

We have helpers for these in drivers/of/of_mtd.c.

> + chip->ecc.bytes = ((fls(1+8*chip->ecc.size)*
> + chip->ecc.strength)/8);

Do you actually need this? I think nand_scan_tail() calculates this for
you.

> + return 0;
> + }
> +
> + if (!strncmp("soft", ecc_mode, 4)) {
> + chip->ecc.mode = NAND_ECC_SOFT;

There's a helper for this too.

> + return 0;
> + }
> + }

In fact, I think most of this could be superseded by using this patch:

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

Then you can assign

chip->dn = node;

and maybe validate a few parameters in this driver to make sure
everything is sane.

>
> /* If CS Base Register selects full hardware ECC then use it */
> if ((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) ==
> BR_DECC_CHK_GEN) {
> + chip->ecc.write_subpage = fsl_elbc_write_subpage;
> chip->ecc.mode = NAND_ECC_HW;
> /* put in small page settings and adjust later if needed */
> chip->ecc.layout = (priv->fmr & FMR_ECCM) ?

Brian

2015-05-21 11:46:39

by Tomas Hlavacek

[permalink] [raw]
Subject: [PATCH v2] mtd: fsl_elbc_nand Add ECC mode selection in DT

Add device tree pointer to chip structure in order to allow turn off the
HW ECC and force own ECC mode and ECC parameters. Newly supported entries
are as per documentation: nand-ecc-mode, nand-ecc-step-size
and nand-ecc-strength.

Add RNDOUT operation which is required for SOFT and SOFT_BCH modes.

Do not set write_subpage function pointer from the driver when it
initializes in SOFT and SOFT_BCH modes.

Signed-off-by: Tomas Hlavacek <[email protected]>
---
drivers/mtd/nand/fsl_elbc_nand.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c
index 04b22fd..b6cdda6 100644
--- a/drivers/mtd/nand/fsl_elbc_nand.c
+++ b/drivers/mtd/nand/fsl_elbc_nand.c
@@ -335,6 +335,14 @@ static void fsl_elbc_cmdfunc(struct mtd_info *mtd, unsigned int command,
fsl_elbc_run_command(mtd);
return;

+ case NAND_CMD_RNDOUT:
+ dev_vdbg(priv->dev,
+ "fsl_elbc_cmdfunc: NAND_CMD_RNDOUT, column: 0x%x.\n",
+ column);
+
+ elbc_fcm_ctrl->index = column;
+ return;
+
/* READOOB reads only the OOB because no ECC is performed. */
case NAND_CMD_READOOB:
dev_vdbg(priv->dev,
@@ -656,6 +664,10 @@ static int fsl_elbc_chip_init_tail(struct mtd_info *mtd)
chip->ecc.steps);
dev_dbg(priv->dev, "fsl_elbc_init: nand->ecc.bytes = %d\n",
chip->ecc.bytes);
+ dev_dbg(priv->dev, "fsl_elbc_init: nand->ecc.size = %d\n",
+ chip->ecc.size);
+ dev_dbg(priv->dev, "fsl_elbc_init: nand->ecc.strength = %d\n",
+ chip->ecc.strength);
dev_dbg(priv->dev, "fsl_elbc_init: nand->ecc.total = %d\n",
chip->ecc.total);
dev_dbg(priv->dev, "fsl_elbc_init: nand->ecc.layout = %p\n",
@@ -677,8 +689,8 @@ static int fsl_elbc_chip_init_tail(struct mtd_info *mtd)
priv->page_size = 1;
setbits32(&lbc->bank[priv->bank].or, OR_FCM_PGS);
/* adjust ecc setup if needed */
- if ((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) ==
- BR_DECC_CHK_GEN) {
+ if (((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) ==
+ BR_DECC_CHK_GEN) && (chip->ecc.mode == NAND_ECC_HW)) {
chip->ecc.size = 512;
chip->ecc.layout = (priv->fmr & FMR_ECCM) ?
&fsl_elbc_oob_lp_eccm1 :
@@ -742,6 +754,7 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)
struct fsl_lbc_regs __iomem *lbc = ctrl->regs;
struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = ctrl->nand;
struct nand_chip *chip = &priv->chip;
+ struct device_node *node = priv->dev->of_node;

dev_dbg(priv->dev, "eLBC Set Information for bank %d\n", priv->bank);

@@ -749,6 +762,9 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)
priv->mtd.priv = chip;
priv->mtd.owner = THIS_MODULE;

+ /* Fill in OF node */
+ chip->dn = node;
+
/* set timeout to maximum */
priv->fmr = 15 << FMR_CWTO_SHIFT;
if (in_be32(&lbc->bank[priv->bank].or) & OR_FCM_PGS)
@@ -774,7 +790,6 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)

chip->ecc.read_page = fsl_elbc_read_page;
chip->ecc.write_page = fsl_elbc_write_page;
- chip->ecc.write_subpage = fsl_elbc_write_subpage;

/* If CS Base Register selects full hardware ECC then use it */
if ((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) ==
@@ -786,6 +801,7 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)
chip->ecc.size = 512;
chip->ecc.bytes = 3;
chip->ecc.strength = 1;
+ chip->ecc.write_subpage = fsl_elbc_write_subpage;
} else {
/* otherwise fall back to default software ECC */
chip->ecc.mode = NAND_ECC_SOFT;
--
2.1.4

2015-05-21 20:59:06

by Scott Wood

[permalink] [raw]
Subject: Re: [PATCH v2] mtd: fsl_elbc_nand Add ECC mode selection in DT

On Thu, 2015-05-21 at 13:46 +0200, Tomas Hlavacek wrote:
> Add device tree pointer to chip structure in order to allow turn off the
> HW ECC and force own ECC mode and ECC parameters. Newly supported entries
> are as per documentation: nand-ecc-mode, nand-ecc-step-size
> and nand-ecc-strength.

If you're using these properties with eLBC, the eLBC binding needs to be
updated to say that they're allowed for use with software ECC.

> @@ -677,8 +689,8 @@ static int fsl_elbc_chip_init_tail(struct mtd_info *mtd)
> priv->page_size = 1;
> setbits32(&lbc->bank[priv->bank].or, OR_FCM_PGS);
> /* adjust ecc setup if needed */
> - if ((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) ==
> - BR_DECC_CHK_GEN) {
> + if (((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) ==
> + BR_DECC_CHK_GEN) && (chip->ecc.mode == NAND_ECC_HW)) {

There's no need to check both. If you're selecting software ECC in the
device tree then you must also remove ECC checking from BRn.

If you want to make Linux write to BRn to ensure that it's consistent
with ecc.mode after the DT init, fine, but don't just silently proceed
without fixing it if there's an inconsistency.

> @@ -786,6 +801,7 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)
> chip->ecc.size = 512;
> chip->ecc.bytes = 3;
> chip->ecc.strength = 1;
> + chip->ecc.write_subpage = fsl_elbc_write_subpage;
> } else {
> /* otherwise fall back to default software ECC */
> chip->ecc.mode = NAND_ECC_SOFT;

...but here you are relying on BRn to be set correctly before
nand_dt_init(), so I'm even more confused by what the previous change is
for.

-Scott