[PATCH v4 0/6] Export the ECC step size to user applications

[PATCH v4 0/6] Export the ECC step size to user applications

[Huang Shijie]

In order to implement the NAND boot for some Freescale's chips, such as
imx23/imx28/imx50/imx6, we use a tool (called kobs-ng) to burn the uboot
and some metadata to nand chip. And the ROM code will use the metadata to
configrate the BCH, and to find the uboot.

The ECC information(ecc step size, ecc strength) which is used to configrate
the BCH is part of the metadata. The kobs-ng can gets the ecc strength from
the sys node /sys/*/mtdX/ecc_strength now. But it can't gets the ecc step size.

This patch set is used to export the ecc step size to user applications.
With this patch set, the kobs-ng can gets the ecc step size now.

v3 --> v4:
	[1] rename the ecc_step to ecc_step_size.

v2 --> v3:
	[1] replace the nand_scan() with nand_scan_ident()/nand_scan_tail(),
	    Let the MTD layer to do the initialization for the ECC info.
	    removed some hack code.
	[2] move the ONFI nand's hooks in nand_set_defaults().
	[3] change the comments.
	[4] misc	

v1 --> v2:
	[1] rename the ecc_size to ecc_step.
	[2] rebase on the latest l2-mtd.

Huang Shijie (6):
  mtd: add a new field to mtd_info{}
  mtd: add a new sys node to show the ecc step size
  mtd: set the ecc step size for master/slave mtd_info
  mtd: set ONFI nand's default hooks in nand_set_defaults()
  mtd: gpmi: remove the nand_scan()
  mtd: update the ABI document about the ecc step size

 Documentation/ABI/testing/sysfs-class-mtd |   10 ++++
 drivers/mtd/mtdcore.c                     |   11 +++++
 drivers/mtd/mtdpart.c                     |    1 +
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c    |   65 +++++++++++++++-------------
 drivers/mtd/nand/nand_base.c              |   13 +++---
 include/linux/mtd/mtd.h                   |    3 +
 6 files changed, 67 insertions(+), 36 deletions(-)

[PATCH v4 1/6] mtd: add a new field to mtd_info{}

[Huang Shijie]

In order to implement the NAND boot for some Freescale's chips, such as
imx23/imx28/imx50/imx6, we use a tool (called kobs-ng) to burn the uboot
and some metadata to nand chip. And the ROM code will use the metadata to
configrate the BCH, and to find the uboot.

The ECC information(ecc step size, ecc strength) which is used to configrate
the BCH is part of the metadata. The kobs-ng can gets the ecc strength from
the sys node /sys/*/ecc_strength now. But it can not gets the ecc step size.

This patch adds a new field to store the ecc step size in mtd_info{}, and
it makes preparation for the next patches.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 include/linux/mtd/mtd.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index a5cf4e8..f9bfe52 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -173,6 +173,9 @@ struct mtd_info {
 	/* ECC layout structure pointer - read only! */
 	struct nand_ecclayout *ecclayout;
 
+	/* the ecc step size. */
+	unsigned int ecc_step_size;
+
 	/* max number of correctible bit errors per ecc step */
 	unsigned int ecc_strength;
 
-- 
1.7.1

[PATCH v4 2/6] mtd: add a new sys node to show the ecc step size

[Huang Shijie]

Add a new sys node to show the ecc step size.
The application then can uses this node to get the ecc step
size.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/mtdcore.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 048c823..5e14d54 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -285,6 +285,16 @@ static DEVICE_ATTR(bitflip_threshold, S_IRUGO | S_IWUSR,
 		   mtd_bitflip_threshold_show,
 		   mtd_bitflip_threshold_store);
 
+static ssize_t mtd_ecc_step_size_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct mtd_info *mtd = dev_get_drvdata(dev);
+
+	return snprintf(buf, PAGE_SIZE, "%u\n", mtd->ecc_step_size);
+
+}
+static DEVICE_ATTR(ecc_step_size, S_IRUGO, mtd_ecc_step_size_show, NULL);
+
 static struct attribute *mtd_attrs[] = {
 	&dev_attr_type.attr,
 	&dev_attr_flags.attr,
@@ -296,6 +306,7 @@ static struct attribute *mtd_attrs[] = {
 	&dev_attr_numeraseregions.attr,
 	&dev_attr_name.attr,
 	&dev_attr_ecc_strength.attr,
+	&dev_attr_ecc_step_size.attr,
 	&dev_attr_bitflip_threshold.attr,
 	NULL,
 };
-- 
1.7.1

[PATCH v4 3/6] mtd: set the ecc step size for master/slave mtd_info

[Huang Shijie]

Set the ecc step size for master/slave mtd_info{}.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/mtdpart.c        |    1 +
 drivers/mtd/nand/nand_base.c |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 3014933..6e732c3 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -516,6 +516,7 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
 	}
 
 	slave->mtd.ecclayout = master->ecclayout;
+	slave->mtd.ecc_step_size = master->ecc_step_size;
 	slave->mtd.ecc_strength = master->ecc_strength;
 	slave->mtd.bitflip_threshold = master->bitflip_threshold;
 
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 8f04fb0..fa35699 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3788,6 +3788,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 	/* propagate ecc info to mtd_info */
 	mtd->ecclayout = chip->ecc.layout;
 	mtd->ecc_strength = chip->ecc.strength;
+	mtd->ecc_step_size = chip->ecc.size;
 	/*
 	 * Initialize bitflip_threshold to its default prior scan_bbt() call.
 	 * scan_bbt() might invoke mtd_read(), thus bitflip_threshold must be
-- 
1.7.1

[PATCH v4 4/6] mtd: set ONFI nand's default hooks in nand_set_defaults()

[Huang Shijie]

We may do some ONFI get/set features operations before we call the
nand_scan_tail().

So move the default ONFI nand hooks into nand_set_defaults().

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/nand/nand_base.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index fa35699..61a7d14 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2794,6 +2794,12 @@ static void nand_set_defaults(struct nand_chip *chip, int busw)
 	if (!chip->select_chip)
 		chip->select_chip = nand_select_chip;
 
+	/* set for ONFI nand */
+	if (!chip->onfi_set_features)
+		chip->onfi_set_features = nand_onfi_set_features;
+	if (!chip->onfi_get_features)
+		chip->onfi_get_features = nand_onfi_get_features;
+
 	/* If called twice, pointers that depend on busw may need to be reset */
 	if (!chip->read_byte || chip->read_byte == nand_read_byte)
 		chip->read_byte = busw ? nand_read_byte16 : nand_read_byte;
@@ -3560,12 +3566,6 @@ int nand_scan_tail(struct mtd_info *mtd)
 	if (!chip->write_page)
 		chip->write_page = nand_write_page;
 
-	/* set for ONFI nand */
-	if (!chip->onfi_set_features)
-		chip->onfi_set_features = nand_onfi_set_features;
-	if (!chip->onfi_get_features)
-		chip->onfi_get_features = nand_onfi_get_features;
-
 	/*
 	 * Check ECC mode, default to software if 3byte/512byte hardware ECC is
 	 * selected and we have 256 byte pagesize fallback to software ECC
-- 
1.7.1

Re: [PATCH v4 4/6] mtd: set ONFI nand's default hooks in nand_set_defaults()

[Brian Norris]

On Fri, Aug 16, 2013 at 10:10:07AM +0800, Huang Shijie wrote:
> We may do some ONFI get/set features operations before we call the
> nand_scan_tail().
> 
> So move the default ONFI nand hooks into nand_set_defaults().
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>

This patch looks similar to a one forwarded by Brian Foster/Florian
Fainelli a while back. But since I don't have a good copy of the other
one, I'll take this.

(BTW, this patch is new for the v4 series, but I don't mind this time)

> ---
>  drivers/mtd/nand/nand_base.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index fa35699..61a7d14 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -2794,6 +2794,12 @@ static void nand_set_defaults(struct nand_chip *chip, int busw)
>  	if (!chip->select_chip)
>  		chip->select_chip = nand_select_chip;
>  
> +	/* set for ONFI nand */
> +	if (!chip->onfi_set_features)
> +		chip->onfi_set_features = nand_onfi_set_features;
> +	if (!chip->onfi_get_features)
> +		chip->onfi_get_features = nand_onfi_get_features;
> +
>  	/* If called twice, pointers that depend on busw may need to be reset */
>  	if (!chip->read_byte || chip->read_byte == nand_read_byte)
>  		chip->read_byte = busw ? nand_read_byte16 : nand_read_byte;
> @@ -3560,12 +3566,6 @@ int nand_scan_tail(struct mtd_info *mtd)
>  	if (!chip->write_page)
>  		chip->write_page = nand_write_page;
>  
> -	/* set for ONFI nand */
> -	if (!chip->onfi_set_features)
> -		chip->onfi_set_features = nand_onfi_set_features;
> -	if (!chip->onfi_get_features)
> -		chip->onfi_get_features = nand_onfi_get_features;
> -
>  	/*
>  	 * Check ECC mode, default to software if 3byte/512byte hardware ECC is
>  	 * selected and we have 256 byte pagesize fallback to software ECC

Brian

Re: [PATCH v4 4/6] mtd: set ONFI nand's default hooks in nand_set_defaults()

[Brian Foster]

On Saturday 17-August-2013 10:55:07 Brian Norris wrote:
> On Fri, Aug 16, 2013 at 10:10:07AM +0800, Huang Shijie wrote:
> > We may do some ONFI get/set features operations before we call the
> > nand_scan_tail().
> > 
> > So move the default ONFI nand hooks into nand_set_defaults().
> > 
> > Signed-off-by: Huang Shijie <b32955@freescale.com>
> 
> This patch looks similar to a one forwarded by Brian Foster/Florian
> Fainelli a while back. But since I don't have a good copy of the other
> one, I'll take this.

 This looks Ok to me.
 Apologies for not sending a “good copy”
 of the earlier one.  Sorry!

cheers!
	-blf-

-- 
Brian Foster
Principal MTS, Software        |  La Ciotat, France
Maxim Integrated               |  http://www.maximintegrated.com/

Re: [PATCH v4 4/6] mtd: set ONFI nand's default hooks in nand_set_defaults()

[Brian Norris]

On Mon, Aug 19, 2013 at 1:06 AM, Brian Foster
<brian.foster@maximintegrated.com> wrote:
> On Saturday 17-August-2013 10:55:07 Brian Norris wrote:
>> On Fri, Aug 16, 2013 at 10:10:07AM +0800, Huang Shijie wrote:
>> > We may do some ONFI get/set features operations before we call the
>> > nand_scan_tail().
>> >
>> > So move the default ONFI nand hooks into nand_set_defaults().
>> >
>> > Signed-off-by: Huang Shijie <b32955@freescale.com>
>>
>> This patch looks similar to a one forwarded by Brian Foster/Florian
>> Fainelli a while back. But since I don't have a good copy of the other
>> one, I'll take this.
>
>  This looks Ok to me.
>  Apologies for not sending a “good copy”
>  of the earlier one.  Sorry!

No problem. Artem had replied about resending it inline (that is the
typical way to review patches), but I was going to get around to
applying it anyway.

Regards,
Brian

Re: [PATCH v4 4/6] mtd: set ONFI nand's default hooks in nand_set_defaults()

[Brian Foster]

On Monday 19-August-2013 17:41:17 Brian Norris wrote:
> On Mon, Aug 19, 2013 at 1:06 AM, Brian Foster
> <brian.foster@maximintegrated.com> wrote:
> > On Saturday 17-August-2013 10:55:07 Brian Norris wrote:
> >> On Fri, Aug 16, 2013 at 10:10:07AM +0800, Huang Shijie wrote:
> >> > We may do some ONFI get/set features operations before we call the
> >> > nand_scan_tail().
>[...]
> >  Apologies for not sending a “good copy”
> >  of the earlier one.  Sorry!
> 
> No problem. Artem had replied about resending it inline (that is the
> typical way to review patches), but I was going to get around to
> applying it anyway.

 Yes, I saw Artem's reply, and I also know Patches are
 normally sent inline.  However, to do that without it
 being corrupted (and other silly problems) from $work
 is tedious and error-prone (I omit the gory details),
 plus I wasn't available (holidays, &tc).  In any case
 the improvement is now progressing, despite the route
 to get to this point being rather strange!

cheers,
	-blf-

-- 
Brian Foster
Principal MTS, Software        |  La Ciotat, France
Maxim Integrated               |  http://www.maximintegrated.com/

[PATCH v4 5/6] mtd: gpmi: remove the nand_scan()

[Huang Shijie]

In order to make the nand_scan() work, the current code uses the hack code
to init the @nand_chip->ecc.size and the @nand_chip->ecc.strength. and
re-init some the ECC info in the gpmi_pre_bbt_scan().
This code is really a little ugly.

The patch does following changes:
  (1) Use the nand_scan_ident()/nand_scan_tail() to replace the nand_scan().

  (2) Init all the necessary values in the gpmi_init_last()
      before we call the nand_scan_tail().

  (3) remove the code setting the ECC info, let the mtd layer to do the
      real job.

  (4) remove the gpmi_scan_bbt(). we do not need this function any more.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   65 +++++++++++++++++---------------
 1 files changed, 35 insertions(+), 30 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index c6b356c..9c89e80 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -214,7 +214,6 @@ static bool set_geometry_by_ecc_info(struct gpmi_nand_data *this)
 	if (geo->page_size < mtd->writesize + mtd->oobsize) {
 		of->offset = geo->page_size - mtd->writesize;
 		of->length = mtd->oobsize - of->offset;
-		mtd->oobavail = gpmi_hw_ecclayout.oobavail = of->length;
 	}
 
 	geo->payload_size = mtd->writesize;
@@ -1582,19 +1581,22 @@ static int gpmi_pre_bbt_scan(struct gpmi_nand_data  *this)
 	if (ret)
 		return ret;
 
-	/* Adjust the ECC strength according to the chip. */
-	this->nand.ecc.strength = this->bch_geometry.ecc_strength;
-	this->mtd.ecc_strength = this->bch_geometry.ecc_strength;
-	this->mtd.bitflip_threshold = this->bch_geometry.ecc_strength;
-
 	/* NAND boot init, depends on the gpmi_set_geometry(). */
 	return nand_boot_init(this);
 }
 
-static int gpmi_scan_bbt(struct mtd_info *mtd)
+static void gpmi_nfc_exit(struct gpmi_nand_data *this)
+{
+	nand_release(&this->mtd);
+	gpmi_free_dma_buffer(this);
+}
+
+static int gpmi_init_last(struct gpmi_nand_data *this)
 {
+	struct mtd_info *mtd = &this->mtd;
 	struct nand_chip *chip = mtd->priv;
-	struct gpmi_nand_data *this = chip->priv;
+	struct nand_ecc_ctrl *ecc = &chip->ecc;
+	struct bch_geometry *bch_geo = &this->bch_geometry;
 	int ret;
 
 	/* Prepare for the BBT scan. */
@@ -1602,6 +1604,16 @@ static int gpmi_scan_bbt(struct mtd_info *mtd)
 	if (ret)
 		return ret;
 
+	/* Init the nand_ecc_ctrl{} */
+	ecc->read_page	= gpmi_ecc_read_page;
+	ecc->write_page	= gpmi_ecc_write_page;
+	ecc->read_oob	= gpmi_ecc_read_oob;
+	ecc->write_oob	= gpmi_ecc_write_oob;
+	ecc->mode	= NAND_ECC_HW;
+	ecc->size	= bch_geo->ecc_chunk_size;
+	ecc->strength	= bch_geo->ecc_strength;
+	ecc->layout	= &gpmi_hw_ecclayout;
+
 	/*
 	 * Can we enable the extra features? such as EDO or Sync mode.
 	 *
@@ -1610,14 +1622,7 @@ static int gpmi_scan_bbt(struct mtd_info *mtd)
 	 */
 	gpmi_extra_init(this);
 
-	/* use the default BBT implementation */
-	return nand_default_bbt(mtd);
-}
-
-static void gpmi_nfc_exit(struct gpmi_nand_data *this)
-{
-	nand_release(&this->mtd);
-	gpmi_free_dma_buffer(this);
+	return 0;
 }
 
 static int gpmi_nfc_init(struct gpmi_nand_data *this)
@@ -1643,33 +1648,33 @@ static int gpmi_nfc_init(struct gpmi_nand_data *this)
 	chip->read_byte		= gpmi_read_byte;
 	chip->read_buf		= gpmi_read_buf;
 	chip->write_buf		= gpmi_write_buf;
-	chip->ecc.read_page	= gpmi_ecc_read_page;
-	chip->ecc.write_page	= gpmi_ecc_write_page;
-	chip->ecc.read_oob	= gpmi_ecc_read_oob;
-	chip->ecc.write_oob	= gpmi_ecc_write_oob;
-	chip->scan_bbt		= gpmi_scan_bbt;
 	chip->badblock_pattern	= &gpmi_bbt_descr;
 	chip->block_markbad	= gpmi_block_markbad;
 	chip->options		|= NAND_NO_SUBPAGE_WRITE;
-	chip->ecc.mode		= NAND_ECC_HW;
-	chip->ecc.size		= 1;
-	chip->ecc.strength	= 8;
-	chip->ecc.layout	= &gpmi_hw_ecclayout;
 	if (of_get_nand_on_flash_bbt(this->dev->of_node))
 		chip->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
 
-	/* Allocate a temporary DMA buffer for reading ID in the nand_scan() */
+	/*
+	 * Allocate a temporary DMA buffer for reading ID in the
+	 * nand_scan_ident().
+	 */
 	this->bch_geometry.payload_size = 1024;
 	this->bch_geometry.auxiliary_size = 128;
 	ret = gpmi_alloc_dma_buffer(this);
 	if (ret)
 		goto err_out;
 
-	ret = nand_scan(mtd, 1);
-	if (ret) {
-		pr_err("Chip scan failed\n");
+	ret = nand_scan_ident(mtd, 1, NULL);
+	if (ret)
+		goto err_out;
+
+	ret = gpmi_init_last(this);
+	if (ret)
+		goto err_out;
+
+	ret = nand_scan_tail(mtd);
+	if (ret)
 		goto err_out;
-	}
 
 	ppdata.of_node = this->pdev->dev.of_node;
 	ret = mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0);
-- 
1.7.1

[PATCH v4 6/6] mtd: update the ABI document about the ecc step size

[Huang Shijie]

We add a new sys node for ecc step size. So update the ABI document about it.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 Documentation/ABI/testing/sysfs-class-mtd |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-mtd b/Documentation/ABI/testing/sysfs-class-mtd
index 3105644..da112ab 100644
--- a/Documentation/ABI/testing/sysfs-class-mtd
+++ b/Documentation/ABI/testing/sysfs-class-mtd
@@ -173,3 +173,13 @@ Description:
 		This is generally applicable only to NAND flash devices with ECC
 		capability.  It is ignored on devices lacking ECC capability;
 		i.e., devices for which ecc_strength is zero.
+
+What:		/sys/class/mtd/mtdX/ecc_step_size
+Date:		May 2013
+KernelVersion:	3.10
+Contact:	linux-mtd@lists.infradead.org
+Description:
+		The size of each ECC step which is used for ECC.
+		Note that some devices will have multiple ecc steps within each
+		writesize region. See more in the ecc_strength above. This will
+		always be a non-negative integer.
-- 
1.7.1

Re: [PATCH v4 6/6] mtd: update the ABI document about the ecc step size

[Artem Bityutskiy]

On Fri, 2013-08-16 at 10:10 +0800, Huang Shijie wrote:
> +
> +What:          /sys/class/mtd/mtdX/ecc_step_size
> +Date:          May 2013
> +KernelVersion: 3.10
> +Contact:       linux-mtd@lists.infradead.org
> +Description:
> +               The size of each ECC step which is used for ECC.
> +               Note that some devices will have multiple ecc steps within each
> +               writesize region.

Actually this phrase is a bit confusing because it may be interpreted as
that one write-size may have ECC steps of multiple sizes. Would you
re-phrase, may be?

Otherwise the patch-set looks good, thanks, but I am not merging the
patches to let Brian review.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH v4 6/6] mtd: update the ABI document about the ecc step size

[Huang Shijie]

On Fri, Aug 16, 2013 at 04:45:59PM +0300, Artem Bityutskiy wrote:
> On Fri, 2013-08-16 at 10:10 +0800, Huang Shijie wrote:
> > +
> > +What:          /sys/class/mtd/mtdX/ecc_step_size
> > +Date:          May 2013
> > +KernelVersion: 3.10
> > +Contact:       linux-mtd@lists.infradead.org
> > +Description:
> > +               The size of each ECC step which is used for ECC.
> > +               Note that some devices will have multiple ecc steps within each
> > +               writesize region.
> 
> Actually this phrase is a bit confusing because it may be interpreted as
> that one write-size may have ECC steps of multiple sizes. Would you
> re-phrase, may be?
What's about the following:
 -----------------------------------------------------------------
   The size of each ECC step which is used for ECC.
   Note that some devices will have multiple ecc steps within each
   writesize region, and the ecc steps share the same size.
 -----------------------------------------------------------------

thanks
Huang Shijie

Re: [PATCH v4 6/6] mtd: update the ABI document about the ecc step size

[Brian Norris]

On Fri, Aug 16, 2013 at 11:26:47PM -0400, Huang Shijie wrote:
> On Fri, Aug 16, 2013 at 04:45:59PM +0300, Artem Bityutskiy wrote:
> > On Fri, 2013-08-16 at 10:10 +0800, Huang Shijie wrote:
> > > +
> > > +What:          /sys/class/mtd/mtdX/ecc_step_size
> > > +Date:          May 2013
> > > +KernelVersion: 3.10
> > > +Contact:       linux-mtd@lists.infradead.org
> > > +Description:
> > > +               The size of each ECC step which is used for ECC.
> > > +               Note that some devices will have multiple ecc steps within each
> > > +               writesize region.
> > 
> > Actually this phrase is a bit confusing because it may be interpreted as
> > that one write-size may have ECC steps of multiple sizes. Would you
> > re-phrase, may be?
> What's about the following:
>  -----------------------------------------------------------------
>    The size of each ECC step which is used for ECC.
>    Note that some devices will have multiple ecc steps within each
>    writesize region, and the ecc steps share the same size.
>  -----------------------------------------------------------------

I took pieces of your message and rewrote it myself. Diff pasted below
(I edited ecc_strength to be less redundant and added a few details that
were worth mentioning). Let me know if you want to revise it, but I'll
push it to l2-mtd.git.

Brian

---
 Documentation/ABI/testing/sysfs-class-mtd | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-mtd b/Documentation/ABI/testing/sysfs-class-mtd
index 3105644..a795582 100644
--- a/Documentation/ABI/testing/sysfs-class-mtd
+++ b/Documentation/ABI/testing/sysfs-class-mtd
@@ -128,9 +128,8 @@ KernelVersion:	3.4
 Contact:	linux-mtd@lists.infradead.org
 Description:
 		Maximum number of bit errors that the device is capable of
-		correcting within each region covering an ecc step.  This will
-		always be a non-negative integer.  Note that some devices will
-		have multiple ecc steps within each writesize region.
+		correcting within each region covering an ECC step (see
+		ecc_step_size).  This will always be a non-negative integer.
 
 		In the case of devices lacking any ECC capability, it is 0.
 
@@ -173,3 +172,16 @@ Description:
 		This is generally applicable only to NAND flash devices with ECC
 		capability.  It is ignored on devices lacking ECC capability;
 		i.e., devices for which ecc_strength is zero.
+
+What:		/sys/class/mtd/mtdX/ecc_step_size
+Date:		May 2013
+KernelVersion:	3.10
+Contact:	linux-mtd@lists.infradead.org
+Description:
+		The size of a single region covered by ECC, known as the ECC
+		step.  Devices may have several equally sized ECC steps within
+		each writesize region.  The step size counts only the data area,
+		not the spare area.
+
+		It will always be a non-negative integer.  In the case of
+		devices lacking any ECC capability, it is 0.

Re: [PATCH v4 6/6] mtd: update the ABI document about the ecc step size

[Huang Shijie]

On Sat, Aug 17, 2013 at 11:14:47AM -0700, Brian Norris wrote:
> On Fri, Aug 16, 2013 at 11:26:47PM -0400, Huang Shijie wrote:
> 
> I took pieces of your message and rewrote it myself. Diff pasted below
> (I edited ecc_strength to be less redundant and added a few details that
> were worth mentioning). Let me know if you want to revise it, but I'll

thanks a lot!

I really appreciate it. I always feel embarrassed when i descibe
something in English :(


> push it to l2-mtd.git.
> 
> Brian
> 
> ---
>  Documentation/ABI/testing/sysfs-class-mtd | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-mtd b/Documentation/ABI/testing/sysfs-class-mtd
> index 3105644..a795582 100644
> --- a/Documentation/ABI/testing/sysfs-class-mtd
> +++ b/Documentation/ABI/testing/sysfs-class-mtd
> @@ -128,9 +128,8 @@ KernelVersion:	3.4
>  Contact:	linux-mtd@lists.infradead.org
>  Description:
>  		Maximum number of bit errors that the device is capable of
> -		correcting within each region covering an ecc step.  This will
> -		always be a non-negative integer.  Note that some devices will
> -		have multiple ecc steps within each writesize region.
> +		correcting within each region covering an ECC step (see
> +		ecc_step_size).  This will always be a non-negative integer.
>  
>  		In the case of devices lacking any ECC capability, it is 0.
>  
> @@ -173,3 +172,16 @@ Description:
>  		This is generally applicable only to NAND flash devices with ECC
>  		capability.  It is ignored on devices lacking ECC capability;
>  		i.e., devices for which ecc_strength is zero.
> +
> +What:		/sys/class/mtd/mtdX/ecc_step_size
> +Date:		May 2013
> +KernelVersion:	3.10
> +Contact:	linux-mtd@lists.infradead.org
> +Description:
> +		The size of a single region covered by ECC, known as the ECC
> +		step.  Devices may have several equally sized ECC steps within
> +		each writesize region.  The step size counts only the data area,
> +		not the spare area.

Maybe this sentence is not accurate enough.

As far as i know, when the gpmi does the hardware ECC, the last ECC step
will use parts of the spare area. Just like:

----------------------------------------------------------------------------------
	 *    |                          P                            |
	 *    |<----------------------------------------------------->|
	 *    |                                                       |
	 *    |                                        (Block Mark)   |
	 *    |                      P'                      |      | |     |
	 *    |<-------------------------------------------->|  D   | |  O' |
	 *    |                                              |<---->| |<--->|
	 *    V                                              V      V V     V
	 *    +---+----------+-+----------+-+----------+-+----------+-+-----+
	 *    | M |   data   |E|   data   |E|   data   |E|   data   |E|     |
	 *    +---+----------+-+----------+-+----------+-+----------+-+-----+
	 *                                                   ^              ^
	 *                                                   |      O       |
	 *                                                   |<------------>|
	 *
	 *	P : the page size for BCH module.
	 *	E : The ECC strength.
	 *	G : the length of Galois Field.
	 *	N : The chunk count of per page.
	 *	M : the metasize of per page.
	 *	C : the ecc chunk size, aka the "data" above.
	 *	P': the nand chip's page size.
	 *	O : the nand chip's oob size.
	 *	O': the free oob.
----------------------------------------------------------------------------------
 In this diagram, the "O" stands for the spare area, and the last ECC
 step will use part of the spare area, the "Block Mark" is the boundary
 for the page and OOB.

thanks
Huang Shijie

Re: [PATCH v4 6/6] mtd: update the ABI document about the ecc step size

[Brian Norris]

On Sun, Aug 18, 2013 at 7:29 AM, Huang Shijie <shijie8@gmail.com> wrote:
> On Sat, Aug 17, 2013 at 11:14:47AM -0700, Brian Norris wrote:
>> On Fri, Aug 16, 2013 at 11:26:47PM -0400, Huang Shijie wrote:
>> I took pieces of your message and rewrote it myself. Diff pasted below
>> (I edited ecc_strength to be less redundant and added a few details that
>> were worth mentioning). Let me know if you want to revise it, but I'll
>
> thanks a lot!
>
> I really appreciate it. I always feel embarrassed when i descibe
> something in English :(

No need to be embarrassed. By this last iteration, it was actually
quite correct; there were just a few issues of style and an extra bit
of redundancy, now that ecc_strength *and* ecc_step_size were
documented.

...
>> @@ -173,3 +172,16 @@ Description:
>>               This is generally applicable only to NAND flash devices with ECC
>>               capability.  It is ignored on devices lacking ECC capability;
>>               i.e., devices for which ecc_strength is zero.
>> +
>> +What:                /sys/class/mtd/mtdX/ecc_step_size
>> +Date:                May 2013
>> +KernelVersion:       3.10
>> +Contact:     linux-mtd@lists.infradead.org
>> +Description:
>> +             The size of a single region covered by ECC, known as the ECC
>> +             step.  Devices may have several equally sized ECC steps within
>> +             each writesize region.  The step size counts only the data area,
>> +             not the spare area.
>
> Maybe this sentence is not accurate enough.
>
> As far as i know, when the gpmi does the hardware ECC, the last ECC step
> will use parts of the spare area. Just like:
>
> ----------------------------------------------------------------------------------
>          *    |                          P                            |
>          *    |<----------------------------------------------------->|
>          *    |                                                       |
>          *    |                                        (Block Mark)   |
>          *    |                      P'                      |      | |     |
>          *    |<-------------------------------------------->|  D   | |  O' |
>          *    |                                              |<---->| |<--->|
>          *    V                                              V      V V     V
>          *    +---+----------+-+----------+-+----------+-+----------+-+-----+
>          *    | M |   data   |E|   data   |E|   data   |E|   data   |E|     |
>          *    +---+----------+-+----------+-+----------+-+----------+-+-----+
>          *                                                   ^              ^
>          *                                                   |      O       |
>          *                                                   |<------------>|
>          *
>          *      P : the page size for BCH module.
>          *      E : The ECC strength.
>          *      G : the length of Galois Field.
>          *      N : The chunk count of per page.
>          *      M : the metasize of per page.
>          *      C : the ecc chunk size, aka the "data" above.
>          *      P': the nand chip's page size.
>          *      O : the nand chip's oob size.
>          *      O': the free oob.
> ----------------------------------------------------------------------------------
>  In this diagram, the "O" stands for the spare area, and the last ECC
>  step will use part of the spare area, the "Block Mark" is the boundary
>  for the page and OOB.

Apparently I totally ignored the possibility that ECC layouts place
their metadata in the data area (the 'E' regions above) not just in
the spare area ('O'). Out of curiosity, is this very common?

The point I really was trying to get to was that the "step" is only
measuring the division of the data area, not of any additional
metadata or spare area. So in your case, I still expect that
ecc_step_size * number-of-steps = writesize
So the step size is still counting "only the data" (not "data + E"),
but its corresponding data is not necessarily placed entirely in the
data area.

I think your objection is correct, and I just rebased and dropped the
inaccurate sentence.

Thanks,
Brian

Re: [PATCH v4 6/6] mtd: update the ABI document about the ecc step size

[Huang Shijie]

于 2013年08月20日 09:02, Brian Norris 写道:
> Apparently I totally ignored the possibility that ECC layouts place
> their metadata in the data area (the 'E' regions above) not just in
> the spare area ('O'). Out of curiosity, is this very common?
>
I think it is not common. AFAIK, only the gpmi uses this layout.

thanks for the rebase.

Huang Shijie

Re: [PATCH v4 0/6] Export the ECC step size to user applications

[Brian Norris]

Hi Huang,

On Fri, Aug 16, 2013 at 10:10:03AM +0800, Huang Shijie wrote:
> In order to implement the NAND boot for some Freescale's chips, such as
> imx23/imx28/imx50/imx6, we use a tool (called kobs-ng) to burn the uboot
> and some metadata to nand chip. And the ROM code will use the metadata to
> configrate the BCH, and to find the uboot.
>
> The ECC information(ecc step size, ecc strength) which is used to configrate
> the BCH is part of the metadata. The kobs-ng can gets the ecc strength from
> the sys node /sys/*/mtdX/ecc_strength now. But it can't gets the ecc step size.
>
> This patch set is used to export the ecc step size to user applications.
> With this patch set, the kobs-ng can gets the ecc step size now.
>
> v3 --> v4:
> [1] rename the ecc_step to ecc_step_size.
>
> v2 --> v3:
> [1] replace the nand_scan() with nand_scan_ident()/nand_scan_tail(),
>    Let the MTD layer to do the initialization for the ECC info.
>    removed some hack code.
> [2] move the ONFI nand's hooks in nand_set_defaults().
> [3] change the comments.
> [4] misc
>
> v1 --> v2:
> [1] rename the ecc_size to ecc_step.
> [2] rebase on the latest l2-mtd.
>
> Huang Shijie (6):
>   mtd: add a new field to mtd_info{}
>   mtd: add a new sys node to show the ecc step size
>   mtd: set the ecc step size for master/slave mtd_info
>   mtd: set ONFI nand's default hooks in nand_set_defaults()
>   mtd: gpmi: remove the nand_scan()
>   mtd: update the ABI document about the ecc step size
>
>  Documentation/ABI/testing/sysfs-class-mtd |   10 ++++
>  drivers/mtd/mtdcore.c                     |   11 +++++
>  drivers/mtd/mtdpart.c                     |    1 +
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c    |   65 +++++++++++++++-------------
>  drivers/mtd/nand/nand_base.c              |   13 +++---
>  include/linux/mtd/mtd.h                   |    3 +
>  6 files changed, 67 insertions(+), 36 deletions(-)

I think we're still missing updates for a few drivers, which probably
shouldn't have mtd->ecc_step_size == 0. For instance,
nand/alauda.c and maybe a few others. Can you take a pass at them,
Huang? Perhaps take a guess at their step size and CC relevant author(s).
If no one cares, then maybe they can stay as zero.

Anyway, I pushed the whole series, including my edits to patch 6. I will
amend if there are more comments.

Thanks,
Brian

Re: [PATCH v4 0/6] Export the ECC step size to user applications

[Huang Shijie]

On Sat, Aug 17, 2013 at 11:35:28AM -0700, Brian Norris wrote:
> Hi Huang,
> 
> On Fri, Aug 16, 2013 at 10:10:03AM +0800, Huang Shijie wrote:
> > In order to implement the NAND boot for some Freescale's chips, such as
> > imx23/imx28/imx50/imx6, we use a tool (called kobs-ng) to burn the uboot
> > and some metadata to nand chip. And the ROM code will use the metadata to
> > configrate the BCH, and to find the uboot.
> >
> > The ECC information(ecc step size, ecc strength) which is used to configrate
> > the BCH is part of the metadata. The kobs-ng can gets the ecc strength from
> > the sys node /sys/*/mtdX/ecc_strength now. But it can't gets the ecc step size.
> >
> > This patch set is used to export the ecc step size to user applications.
> > With this patch set, the kobs-ng can gets the ecc step size now.
> >
> > v3 --> v4:
> > [1] rename the ecc_step to ecc_step_size.
> >
> > v2 --> v3:
> > [1] replace the nand_scan() with nand_scan_ident()/nand_scan_tail(),
> >    Let the MTD layer to do the initialization for the ECC info.
> >    removed some hack code.
> > [2] move the ONFI nand's hooks in nand_set_defaults().
> > [3] change the comments.
> > [4] misc
> >
> > v1 --> v2:
> > [1] rename the ecc_size to ecc_step.
> > [2] rebase on the latest l2-mtd.
> >
> > Huang Shijie (6):
> >   mtd: add a new field to mtd_info{}
> >   mtd: add a new sys node to show the ecc step size
> >   mtd: set the ecc step size for master/slave mtd_info
> >   mtd: set ONFI nand's default hooks in nand_set_defaults()
> >   mtd: gpmi: remove the nand_scan()
> >   mtd: update the ABI document about the ecc step size
> >
> >  Documentation/ABI/testing/sysfs-class-mtd |   10 ++++
> >  drivers/mtd/mtdcore.c                     |   11 +++++
> >  drivers/mtd/mtdpart.c                     |    1 +
> >  drivers/mtd/nand/gpmi-nand/gpmi-nand.c    |   65 +++++++++++++++-------------
> >  drivers/mtd/nand/nand_base.c              |   13 +++---
> >  include/linux/mtd/mtd.h                   |    3 +
> >  6 files changed, 67 insertions(+), 36 deletions(-)
> 
> I think we're still missing updates for a few drivers, which probably
> shouldn't have mtd->ecc_step_size == 0. For instance,
> nand/alauda.c and maybe a few others. Can you take a pass at them,
okay.  I will check it right now, and send them a email.


thanks
Huang Shijie
> Huang? Perhaps take a guess at their step size and CC relevant author(s).
> If no one cares, then maybe they can stay as zero.
> 
> Anyway, I pushed the whole series, including my edits to patch 6. I will
> amend if there are more comments.
> 
> Thanks,
> Brian
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/