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

[PATCH v3 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.

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

 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 v3 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..effdd41 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;
+
 	/* max number of correctible bit errors per ecc step */
 	unsigned int ecc_strength;
 
-- 
1.7.1

[PATCH v3 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..6aa952b 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_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);
+
+}
+static DEVICE_ATTR(ecc_step, S_IRUGO, mtd_ecc_step_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.attr,
 	&dev_attr_bitflip_threshold.attr,
 	NULL,
 };
-- 
1.7.1

[PATCH v3 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..63b42a6 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 = master->ecc_step;
 	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..6e4095d 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 = 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 v3 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 6e4095d..ff605c8 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

[PATCH v3 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 v3 6/6] mtd: update the ABI document about the ecc step

[Huang Shijie]

We add a new sys node for ecc step. 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..038686d 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
+Date:		May 2013
+KernelVersion:	3.10
+Contact:	linux-mtd@lists.infradead.org
+Description:
+		The size of 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 v3 3/6] mtd: set the ecc step size for master/slave mtd_info

[Gupta, Pekon]

> 
> 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..63b42a6 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 = master->ecc_step;
>  	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..6e4095d 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 = chip->ecc.size;

[Pekon]: Sorry for noticing this lately.. 
But do you really want to name ecc.*size* as *ecc_step* ?
Wouldn't it be better to follow same nomenclature everywhere ?
Like ' mtd->ecc_size = chip.ecc.size;'

Because we also have ecc.steps as another parameter here.
And someone might want to expose it in user-space later.
(Just a naming change requested here to keep things in sync..)

with regards, pekon

>  	/*
>  	 * Initialize bitflip_threshold to its default prior scan_bbt() call.
>  	 * scan_bbt() might invoke mtd_read(), thus bitflip_threshold must
> be
> --
> 1.7.1

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

[Huang Shijie]

于 2013年08月12日 15:00, Gupta, Pekon 写道:
>> 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..63b42a6 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 = master->ecc_step;
>>   	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..6e4095d 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 = chip->ecc.size;
> [Pekon]: Sorry for noticing this lately..
> But do you really want to name ecc.*size* as *ecc_step* ?
Please see :
http://lists.infradead.org/pipermail/linux-mtd/2013-May/046928.html

Artem thought the ecc_step is more proper.


> Wouldn't it be better to follow same nomenclature everywhere ?
> Like ' mtd->ecc_size = chip.ecc.size;'
>
> Because we also have ecc.steps as another parameter here.
there is no need to expose the ecc.steps to use space.

Since the user can get the ecc_step, and pagesize, they can get the 
ecc.steps by:
(pagesize / ecc_step).

thanks
Huang Shijie
> And someone might want to expose it in user-space later.
> (Just a naming change requested here to keep things in sync..)

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

[Gupta, Pekon]

> 
> 于 2013年08月12日 15:00, Gupta, Pekon 写道:
> >> 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..63b42a6 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 = master->ecc_step;
> >>   	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..6e4095d 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 = chip->ecc.size;
> > [Pekon]: Sorry for noticing this lately..
> > But do you really want to name ecc.*size* as *ecc_step* ?
> Please see :
> http://lists.infradead.org/pipermail/linux-mtd/2013-May/046928.html
> 
> Artem thought the ecc_step is more proper.
> 
[Pekon]: Following Artem's recommendations.. Can this be named as
chip->ecc.size = ecc_step_size.
then chip->ecc.bytes  = ecc_syndrome_size (or ecc_code_size)

In addition, chip->ecc.bytes should also be helpful for userspace
utility to determine how much bytes to reserve in spare-area for ECC.
So exposing that as sysfs entry is also good.

> 
> > Wouldn't it be better to follow same nomenclature everywhere ?
> > Like ' mtd->ecc_size = chip.ecc.size;'
> >
> > Because we also have ecc.steps as another parameter here.
> there is no need to expose the ecc.steps to use space.
> 
> Since the user can get the ecc_step, and pagesize, they can get the
> ecc.steps by:
> (pagesize / ecc_step).
> 
[Pekon]: Agreed, chip->ecc.steps need not be exposed to user-space.
It was mentioned bcoz ecc_step and chip->ecc.steps look similar.


with regards, pekon

> thanks
> Huang Shijie
> > And someone might want to expose it in user-space later.
> > (Just a naming change requested here to keep things in sync..)

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

[Huang Shijie]

于 2013年08月12日 17:24, Gupta, Pekon 写道:
> [Pekon]: Following Artem's recommendations.. Can this be named as
> chip->ecc.size = ecc_step_size.
> then chip->ecc.bytes  = ecc_syndrome_size (or ecc_code_size)
yes, we can rename these fields in the future with another patch set.


> In addition, chip->ecc.bytes should also be helpful for userspace
> utility to determine how much bytes to reserve in spare-area for ECC.
> So exposing that as sysfs entry is also good.
>
I do not need the chip->ecc.bytes. :)
For me, export the chip->ecc.size is enough.

So you can submit a patch if you need this field.

thanks
Huang Shijie

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

[Brian Norris]

On Mon, Aug 12, 2013 at 05:44:57PM +0800, Huang Shijie wrote:
> 于 2013年08月12日 17:24, Gupta, Pekon 写道:
> >[Pekon]: Following Artem's recommendations.. Can this be named as
> >chip->ecc.size = ecc_step_size.
> >then chip->ecc.bytes  = ecc_syndrome_size (or ecc_code_size)
> yes, we can rename these fields in the future with another patch set.

Internal naming can be changed in the future if needed, but we really
need to get a good sysfs name now, since that becomes ABI. And it would
also be good to get this to be clearly consistent internally too.

So the main decision to be made here: what to name the sysfs field? It
currently stands as ecc_step, but for clarity to users (and for avoiding
internal confusion with nand_chip.ecc.steps), I think ecc_step_size is
just as good or better.

Then, we might as well have the mtd_info field be
mtd_info.ecc_step_size. So we would have the following names.

For ECC step size:

  /sys/class/mtd/mtdX/ecc_step_size
  mtd_info.ecc_step_size
  nand_chip.ecc.size    <--- candidate for renaming to ...ecc.step_size

For ECC strength:

  /sys/class/mtd/mtdX/ecc_strength
  mtd_info.ecc_strength
  nand_chip.ecc.strength

Other fields to consider for internal naming:

  # Number of ECC steps per page
  nand_chip.ecc.steps

  # Number of ECC bytes per ECC step
  nand_chip.ecc.bytes

Please, let's get an agreement on one of these, so we hopefully only
need at most a v4 patch series:

(1) Huang's current patch set (v3)
(2) My proposed naming above
(3) A third option? (AFAICT, (1) or (2) should be satisfactory)

If we go with (2), the v4 patch series only needs to take care of the
new sysfs naming (plus documentation) and the new mtd_info field naming.
Any internal consistency patches (e.g., for nand_ecc_ctrl) can come as
follow-ups.

> >In addition, chip->ecc.bytes should also be helpful for userspace
> >utility to determine how much bytes to reserve in spare-area for ECC.
> >So exposing that as sysfs entry is also good.
> >
> I do not need the chip->ecc.bytes. :)
> For me, export the chip->ecc.size is enough.

A better argument against this is that ecc.bytes does not necessarily
have utility across many types of NAND drivers by itself, since ECC
layouts differ. And at that point, we're trying to duplicate the
behavior of ioctl(ECCGETLAYOUT) (which notably has run out of room and
isn't 100% informative anymore).

> So you can submit a patch if you need this field.

Yes, if you have good reason for exporting it, send a patch and a good
argument. And it would need to explain why we can't just use
ECCGETLAYOUT.

Brian

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

[Gupta, Pekon]

> 
> On Mon, Aug 12, 2013 at 05:44:57PM +0800, Huang Shijie wrote:
> > 于 2013年08月12日 17:24, Gupta, Pekon 写道:
> > >[Pekon]: Following Artem's recommendations.. Can this be named as
> > >chip->ecc.size = ecc_step_size.
> > >then chip->ecc.bytes  = ecc_syndrome_size (or ecc_code_size)
> > yes, we can rename these fields in the future with another patch set.
> 
> Internal naming can be changed in the future if needed, but we really
> need to get a good sysfs name now, since that becomes ABI. And it would
> also be good to get this to be clearly consistent internally too.
> 
> So the main decision to be made here: what to name the sysfs field? It
> currently stands as ecc_step, but for clarity to users (and for avoiding
> internal confusion with nand_chip.ecc.steps), I think ecc_step_size is
> just as good or better.
> 
> Then, we might as well have the mtd_info field be
> mtd_info.ecc_step_size. So we would have the following names.
> 
> For ECC step size:
> 
>   /sys/class/mtd/mtdX/ecc_step_size
>   mtd_info.ecc_step_size
>   nand_chip.ecc.size    <--- candidate for renaming to ...ecc.step_size
> 
Agree.

> For ECC strength:
> 
>   /sys/class/mtd/mtdX/ecc_strength
>   mtd_info.ecc_strength
>   nand_chip.ecc.strength
> 
Agree.

> Other fields to consider for internal naming:
> 
>   # Number of ECC steps per page
>   nand_chip.ecc.steps
mtd_info.ecc_steps  (if needed in mtd_info)

> 
>   # Number of ECC bytes per ECC step
>   nand_chip.ecc.bytes
mtd_info.ecc_code_size
nand_chip.ecc.bytes    <--- candidate for renaming to ...ecc.code_size

> Please, let's get an agreement on one of these, so we hopefully only
> need at most a v4 patch series:
> 
> (1) Huang's current patch set (v3)
> (2) My proposed naming above
> (3) A third option? (AFAICT, (1) or (2) should be satisfactory)
> 
> If we go with (2), the v4 patch series only needs to take care of the
> new sysfs naming (plus documentation) and the new mtd_info field naming.
> Any internal consistency patches (e.g., for nand_ecc_ctrl) can come as
> follow-ups.
> 
> > >In addition, chip->ecc.bytes should also be helpful for userspace
> > >utility to determine how much bytes to reserve in spare-area for ECC.
> > >So exposing that as sysfs entry is also good.
> > >
> > I do not need the chip->ecc.bytes. :)
> > For me, export the chip->ecc.size is enough.
> 
> A better argument against this is that ecc.bytes does not necessarily
> have utility across many types of NAND drivers by itself, since ECC
> layouts differ. And at that point, we're trying to duplicate the
> behavior of ioctl(ECCGETLAYOUT) (which notably has run out of room and
> isn't 100% informative anymore).
> 
> > So you can submit a patch if you need this field.
> 
> Yes, if you have good reason for exporting it, send a patch and a good
> argument. And it would need to explain why we can't just use
> ECCGETLAYOUT.
> 
Thanks for the info..
I wasn't fully aware of ioctl(ECCGETLAYOUT), I'll explore it further.

Also, is there any documentation or URL explaining all ioctl for MTD?

with regards, pekon

> Brian

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

[Huang Shijie]

于 2013年08月13日 12:29, Gupta, Pekon 写道:
> (1) Huang's current patch set (v3)
> >  (2) My proposed naming above
> >  (3) A third option? (AFAICT, (1) or (2) should be satisfactory)
> >  
> >  If we go with (2), the v4 patch series only needs to take care of the
> >  new sysfs naming (plus documentation) and the new mtd_info field naming.
I agree with the renaming, i will wait for several days, and send out 
the v4.

thanks
Huang Shijie

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

[Brian Norris]

On Tue, Aug 13, 2013 at 04:29:46AM +0000, Gupta, Pekon wrote:
> > On Mon, Aug 12, 2013 at 05:44:57PM +0800, Huang Shijie wrote:
> > > 于 2013年08月12日 17:24, Gupta, Pekon 写道:
> > > >In addition, chip->ecc.bytes should also be helpful for userspace
> > > >utility to determine how much bytes to reserve in spare-area for ECC.
> > > >So exposing that as sysfs entry is also good.
> > > >
> > > I do not need the chip->ecc.bytes. :)
> > > For me, export the chip->ecc.size is enough.
> > 
> > A better argument against this is that ecc.bytes does not necessarily
> > have utility across many types of NAND drivers by itself, since ECC
> > layouts differ. And at that point, we're trying to duplicate the
> > behavior of ioctl(ECCGETLAYOUT) (which notably has run out of room and
> > isn't 100% informative anymore).
> > 
> > > So you can submit a patch if you need this field.
> > 
> > Yes, if you have good reason for exporting it, send a patch and a good
> > argument. And it would need to explain why we can't just use
> > ECCGETLAYOUT.
> > 
> Thanks for the info..
> I wasn't fully aware of ioctl(ECCGETLAYOUT), I'll explore it further.

Just to be clear, there may be good reason to duplicate features between
ECCGETLAYOUT and sysfs (for instance, because we've grown beyond the
size of ECCGETLAYOUT, or simply so that modern, useful parameters are
all in one place).

> Also, is there any documentation or URL explaining all ioctl for MTD?

That's a good question. As far as I know, the best reference is
include/uapi/mtd/mtd-abi.h. There's some old documentation for UBI
ioctls on:

  http://linux-mtd.infradead.org/
  http://linux-mtd.infradead.org/doc/ubidesign/ubidesign.pdf

I don't really see anything like that for MTD. But you are free to write
up such documentation! You can send patches against mtd-www.git:

  http://linux-mtd.infradead.org/faq/general.html#L_mtdwww

Or perhaps we could use something within the kernel Documentation/mtd/
directory? I would consult Artem/David for that one.

Brian