[PATCH v2 0/4] Export the ecc step size to user applications

[PATCH v2 0/4] 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.

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

Huang Shijie (4):
  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: gpmi: update the ecc step size for mtd_info{}

 drivers/mtd/mtdcore.c                  |   11 +++++++++++
 drivers/mtd/mtdpart.c                  |    1 +
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c |    1 +
 drivers/mtd/nand/nand_base.c           |    1 +
 include/linux/mtd/mtd.h                |    3 +++
 5 files changed, 17 insertions(+), 0 deletions(-)

[PATCH v2 1/4] 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 v2 2/4] 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 c400c57..63903b9 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, "%lu\n", (unsigned long)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

Re: [PATCH v2 2/4] mtd: add a new sys node to show the ecc step size

[Brian Norris]

On Wed, May 15, 2013 at 04:46:44PM +0800, Huang Shijie wrote:
> 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 c400c57..63903b9 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, "%lu\n", (unsigned long)mtd->ecc_step);

ecc_step is an unsigned int, so why cast to unsigned long? Just use:

	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,

...

Brian

[PATCH v2 3/4] 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 359e105..48dfab5 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3747,6 +3747,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 v2 4/4] mtd: gpmi: update the ecc step size for mtd_info{}

[Huang Shijie]

update the ecc step size when we have already get the right value.

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

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 53180da..bc598e6 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -1598,6 +1598,7 @@ static int gpmi_pre_bbt_scan(struct gpmi_nand_data  *this)
 	/* 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.ecc_step = this->bch_geometry.ecc_chunk_size;
 	this->mtd.bitflip_threshold = this->bch_geometry.ecc_strength;
 
 	/* NAND boot init, depends on the gpmi_set_geometry(). */
-- 
1.7.1

Re: [PATCH v2 4/4] mtd: gpmi: update the ecc step size for mtd_info{}

[Brian Norris]

On Wed, May 15, 2013 at 04:46:46PM +0800, Huang Shijie wrote:
> update the ecc step size when we have already get the right value.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index 53180da..bc598e6 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -1598,6 +1598,7 @@ static int gpmi_pre_bbt_scan(struct gpmi_nand_data  *this)
>  	/* 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.ecc_step = this->bch_geometry.ecc_chunk_size;

Why do you need a special case here for gpmi-nand? GPMI should be
initializing nand_ecc_ctrl.size (this->ecc.size) to something meaningful
instead of directly setting to 1. Then nand_base will take care of
setting mtd.ecc_step.

Along the same line, you shouldn't need to directly override
mtd.ecc_strength here; you should be setting this->ecc.strength
appropriately.

I see several instances here where gpmi-nand is hacking around
nand_base, rather than using its intended infrastructure.

The right way seems to be to avoid nand_scan() directly but instead to
use nand_scan_ident() and nand_scan_tail() separately (that's what
they're exported for) so that you can initialize any geometry-related
options before nand_scan_tail() does the last steps.

In the end, you shouldn't be needing to override this->scan_bbt at all,
since you really intend to use the default implementation; you just are
doing this to hack around your issues. In fact, I think every use of
nand_default_bbt() outside of nand_base is a design mistake.

>  	this->mtd.bitflip_threshold = this->bch_geometry.ecc_strength;
>  
>  	/* NAND boot init, depends on the gpmi_set_geometry(). */
> 

Brian

Re: [PATCH v2 4/4] mtd: gpmi: update the ecc step size for mtd_info{}

[Brian Norris]

On Sat, Aug 10, 2013 at 12:41 AM, Brian Norris
<computersforpeace@gmail.com> wrote:
> On Wed, May 15, 2013 at 04:46:46PM +0800, Huang Shijie wrote:
>> update the ecc step size when we have already get the right value.
>>
>> Signed-off-by: Huang Shijie <b32955@freescale.com>
>> ---
>>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>> index 53180da..bc598e6 100644
>> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>> @@ -1598,6 +1598,7 @@ static int gpmi_pre_bbt_scan(struct gpmi_nand_data  *this)
>>       /* 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.ecc_step = this->bch_geometry.ecc_chunk_size;
>
> Why do you need a special case here for gpmi-nand? GPMI should be
> initializing nand_ecc_ctrl.size (this->ecc.size) to something meaningful

Correction: should be this->nand.ecc.size

> instead of directly setting to 1. Then nand_base will take care of
> setting mtd.ecc_step.
>
> Along the same line, you shouldn't need to directly override
> mtd.ecc_strength here; you should be setting this->ecc.strength
> appropriately.

Correction: should be this->nand.ecc.strength. And another correction:
you are already setting this field, but you are doing it in the wrong
place.

> I see several instances here where gpmi-nand is hacking around
> nand_base, rather than using its intended infrastructure.
>
> The right way seems to be to avoid nand_scan() directly but instead to
> use nand_scan_ident() and nand_scan_tail() separately (that's what
> they're exported for) so that you can initialize any geometry-related
> options before nand_scan_tail() does the last steps.
>
> In the end, you shouldn't be needing to override this->scan_bbt at all,
> since you really intend to use the default implementation; you just are
> doing this to hack around your issues. In fact, I think every use of
> nand_default_bbt() outside of nand_base is a design mistake.
>
>>       this->mtd.bitflip_threshold = this->bch_geometry.ecc_strength;
>>
>>       /* NAND boot init, depends on the gpmi_set_geometry(). */
>>

Sorry for the above mistakes, but the argument still holds. In fact,
it's somewhat stronger, because gpmi-nand has to assign the same
information to two different (redundant) fields
(this->nand.ecc.strength and this->mtd.ecc_strength) because it is not
using the proper layering here.

Brian

Re: [PATCH v2 4/4] mtd: gpmi: update the ecc step size for mtd_info{}

[Huang Shijie]

于 2013年08月10日 03:53, Brian Norris 写道:
> The right way seems to be to avoid nand_scan() directly but instead to
> >  use nand_scan_ident() and nand_scan_tail() separately (that's what
> >  they're exported for) so that you can initialize any geometry-related
> >  options before nand_scan_tail() does the last steps.
i agree with you.
I will create a v3 version tomorrow when i back to office.

thanks for the review.
Huang Shijie

Re: [PATCH v2 0/4] Export the ecc step size to user applications

[Artem Bityutskiy]

On Wed, 2013-05-15 at 16:46 +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.

Looks good to me. Would you please update
Documentation/ABI/testing/sysfs-class-mtd as well?

-- 
Best Regards,
Artem Bityutskiy

[PATCH v2 append] 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 |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-mtd b/Documentation/ABI/testing/sysfs-class-mtd
index 3105644..62a1cda 100644
--- a/Documentation/ABI/testing/sysfs-class-mtd
+++ b/Documentation/ABI/testing/sysfs-class-mtd
@@ -173,3 +173,14 @@ 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 per ecc correction.
+		See more in the ecc_strength above. This will always be a
+		non-negative integer.
+
+		In the case of devices lacking any ECC capability, it is 0.
-- 
1.7.1

Re: [PATCH v2 append] mtd: update the ABI document about the ecc step

[Brian Norris]

On Thu, May 16, 2013 at 12:09:43PM +0800, Huang Shijie wrote:
> 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 |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-mtd b/Documentation/ABI/testing/sysfs-class-mtd
> index 3105644..62a1cda 100644
> --- a/Documentation/ABI/testing/sysfs-class-mtd
> +++ b/Documentation/ABI/testing/sysfs-class-mtd
> @@ -173,3 +173,14 @@ 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 per ecc correction.

Despite the usage of lower-case 'ecc' in some places, I would prefer
consistent usage of upper-case 'ECC'. And "ECC correction" is redundant.

Also, "...which is used for per ecc correction" doesn't make sense to
me. Perhaps you just mean "...which is used for ECC"?

The following note under the current ecc_strength documentation makes
more sense here, actually:

  "Note that some devices will have multiple ecc steps within each
   writesize region."

Maybe just copy-and-paste it here too?

> +		See more in the ecc_strength above. This will always be a
> +		non-negative integer.
> +
> +		In the case of devices lacking any ECC capability, it is 0.

These wording comments aren't highly important. If I have any more
changes, I can just edit before applying when you send v3.

Thanks,
Brian

Re: [PATCH v2 0/4] Export the ecc step size to user applications

[Artem Bityutskiy]

On Wed, 2013-05-15 at 22:19 +0800, Huang Shijie wrote:
> ok.   can i send a separate patch to fix it?
> or a new version patch set？
> 
Just a follow-up patch. I would like to let this patch-set sit in the
mailing list for some time before I pick it, in case someone wants to
comment.

Also, surely the things you expose are the real ECC step and ECC
strength? I think the datasheet ones in your other patch-set should be
named differently to make sure the reader of the code understands that
they are only the recommended datasheet values, not necessarily the
values which are actually in use.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH v2 0/4] Export the ecc step size to user applications

[Huang Shijie]

于 2013年05月16日 15:19, Artem Bityutskiy 写道:
> Also, surely the things you expose are the real ECC step and ECC
> strength? I think the datasheet ones in your other patch-set should be
yes. I am sure of that.
I need the _real_ ECC step and ECC strength.

thanks
Huang Shijie

Re: [PATCH v2 0/4] Export the ecc step size to user applications

[Huang Shijie]

Hi Artem & Brian:
> 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.
>
> v1 --> v2:
> 	[1] rename the ecc_size to ecc_step.
> 	[2] rebase on the latest l2-mtd.
>
> Huang Shijie (4):
>   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: gpmi: update the ecc step size for mtd_info{}
>
>  drivers/mtd/mtdcore.c                  |   11 +++++++++++
>  drivers/mtd/mtdpart.c                  |    1 +
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c |    1 +
>  drivers/mtd/nand/nand_base.c           |    1 +
>  include/linux/mtd/mtd.h                |    3 +++
>  5 files changed, 17 insertions(+), 0 deletions(-)
>
Could you merge this patch set ?

thanks
Huang Shijie