[PATCH] mtd: gpmi: fix the ecc regression

[PATCH] mtd: gpmi: fix the ecc regression

[Huang Shijie]

The patch "2febcdf mtd: gpmi: set the BCHs geometry with the ecc info"
makes the gpmi to use the ECC info provided by the MTD code, and creates
a different NAND ecc layout for the BCH which brings a regression to us:
   We can not mount the ubifs which was created by the old NAND ecc layout.

This patch adds a new property : "fsl,use-mtd-ecc-info".

If we do not enable it, we will use the ecc strength calculated by ourselves
and use all the OOB area (this is the legacy method);

If we enable it, we will use the ecc strength provided by the MTD layer,
and create a new NAND ecc layout which may has free space in the OOB
(for some SLC nand, we may support the JFFS2 with the new NAND ecc layout).

Signed-off-by: Huang Shijie <b32955-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 .../devicetree/bindings/mtd/gpmi-nand.txt          |    4 ++++
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c             |    7 ++++++-
 2 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/gpmi-nand.txt b/Documentation/devicetree/bindings/mtd/gpmi-nand.txt
index 551b2a1..fe84a3d 100644
--- a/Documentation/devicetree/bindings/mtd/gpmi-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/gpmi-nand.txt
@@ -17,6 +17,10 @@ Required properties:
 Optional properties:
   - nand-on-flash-bbt: boolean to enable on flash bbt option if not
                        present false
+  - fsl,use-mtd-ecc-info: By enabling this boolean property, the gpmi can uses
+                       the MTD's ECC info (if the MTD has). So we may have free
+                       space in the OOB area, and we may support the JFFS2 with
+                       some SLC nand, such as Micron's SLC nand.
 
 The device tree may optionally contain sub-nodes describing partitions of the
 address space. See partition.txt for more detail.
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 5a1bbc7..62c0712 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -271,7 +271,9 @@ static int legacy_set_geometry(struct gpmi_nand_data *this)
 		dev_err(this->dev,
 			"We can not support this nand chip."
 			" Its required ecc strength(%d) is beyond our"
-			" capability(%d).\n", geo->ecc_strength,
+			" capability(%d). If you have not enabled the"
+			"'fsl,use-mtd-ecc-info', enable it and try again.\n",
+			geo->ecc_strength,
 			(GPMI_IS_MX6Q(this) ? MX6_ECC_STRENGTH_MAX
 					: MXS_ECC_STRENGTH_MAX));
 		return -EINVAL;
@@ -352,6 +354,9 @@ static int legacy_set_geometry(struct gpmi_nand_data *this)
 
 int common_nfc_set_geometry(struct gpmi_nand_data *this)
 {
+	if (!of_property_read_bool(this->dev->of_node, "fsl,use-mtd-ecc-info"))
+		return legacy_set_geometry(this);
+
 	return set_geometry_by_ecc_info(this) ? 0 : legacy_set_geometry(this);
 }
 
-- 
1.7.2.rc3


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mtd: gpmi: fix the ecc regression

[Mark Rutland]

On Tue, Oct 22, 2013 at 06:10:28AM +0100, Huang Shijie wrote:
> The patch "2febcdf mtd: gpmi: set the BCHs geometry with the ecc info"
> makes the gpmi to use the ECC info provided by the MTD code, and creates
> a different NAND ecc layout for the BCH which brings a regression to us:
>    We can not mount the ubifs which was created by the old NAND ecc layout.
> 
> This patch adds a new property : "fsl,use-mtd-ecc-info".

This name sounds _extremely_ Linux-specific, and that's not the direction I'd
like to see bindings take.

> 
> If we do not enable it, we will use the ecc strength calculated by ourselves
> and use all the OOB area (this is the legacy method);
> 
> If we enable it, we will use the ecc strength provided by the MTD layer,
> and create a new NAND ecc layout which may has free space in the OOB
> (for some SLC nand, we may support the JFFS2 with the new NAND ecc layout).

If the patch you referenced changed the layout without maintainng backwards
compatibility, it sounds like it is broken and has caused a regression.

This is a work around, not a fix.

> 
> Signed-off-by: Huang Shijie <b32955-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  .../devicetree/bindings/mtd/gpmi-nand.txt          |    4 ++++
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c             |    7 ++++++-
>  2 files changed, 10 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/gpmi-nand.txt b/Documentation/devicetree/bindings/mtd/gpmi-nand.txt
> index 551b2a1..fe84a3d 100644
> --- a/Documentation/devicetree/bindings/mtd/gpmi-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/gpmi-nand.txt
> @@ -17,6 +17,10 @@ Required properties:
>  Optional properties:
>    - nand-on-flash-bbt: boolean to enable on flash bbt option if not
>                         present false
> +  - fsl,use-mtd-ecc-info: By enabling this boolean property, the gpmi can uses
> +                       the MTD's ECC info (if the MTD has). So we may have free
> +                       space in the OOB area, and we may support the JFFS2 with
> +                       some SLC nand, such as Micron's SLC nand.

I'm not very familiar with MTDs. Where exactly does this ECC info come from? Is
it data internal to Linux, or is it probed from the device itself?

>  
>  The device tree may optionally contain sub-nodes describing partitions of the
>  address space. See partition.txt for more detail.
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index 5a1bbc7..62c0712 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -271,7 +271,9 @@ static int legacy_set_geometry(struct gpmi_nand_data *this)
>  		dev_err(this->dev,
>  			"We can not support this nand chip."
>  			" Its required ecc strength(%d) is beyond our"
> -			" capability(%d).\n", geo->ecc_strength,
> +			" capability(%d). If you have not enabled the"
> +			"'fsl,use-mtd-ecc-info', enable it and try again.\n",
> +			geo->ecc_strength,

Why are we telling people to do that with no description as to when to do so?
Can we not just fall back to this instead?

>  			(GPMI_IS_MX6Q(this) ? MX6_ECC_STRENGTH_MAX
>  					: MXS_ECC_STRENGTH_MAX));
>  		return -EINVAL;
> @@ -352,6 +354,9 @@ static int legacy_set_geometry(struct gpmi_nand_data *this)
>  
>  int common_nfc_set_geometry(struct gpmi_nand_data *this)
>  {
> +	if (!of_property_read_bool(this->dev->of_node, "fsl,use-mtd-ecc-info"))
> +		return legacy_set_geometry(this);
> +
>  	return set_geometry_by_ecc_info(this) ? 0 : legacy_set_geometry(this);

This looks like it could be reorganised to fall back if necessary.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mtd: gpmi: fix the ecc regression

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 1205 bytes --]

On Tue, 2013-10-22 at 14:34 +0100, Mark Rutland wrote:
> 
> If the patch you referenced changed the layout without maintainng backwards
> compatibility, it sounds like it is broken and has caused a regression.
> 
> This is a work around, not a fix.

Kind of.

Yes, the best answer right now at -rc6 might be to just revert the
functional change and *always* use legacy_set_geometry().

And the name of the proposed new DT property could certainly do with
some improvement.

But fundamentally, the "Linux-specific" bit was already there as an
implicit assumption in the bindings, because there was a de facto
standard for how you lay out the ECC "as legacy_set_geometry() does it".

Now we want to set an option to indicate that it should be automatically
calculated from the properties of the chip (as set_geometry_by_ecc_info
does it), rather than done the old way. And it's not unreasonable to
have that as a DT property. We're no *more* Linux-specific than we were
before.

But perhaps it's too late for 3.12 to do that?

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5745 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: gpmi: fix the ecc regression

[Brian Norris]

Hi Huang,

On Tue, Oct 22, 2013 at 01:10:28PM +0800, Huang Shijie wrote:
> The patch "2febcdf mtd: gpmi: set the BCHs geometry with the ecc info"
> makes the gpmi to use the ECC info provided by the MTD code, and creates
> a different NAND ecc layout for the BCH which brings a regression to us:
>    We can not mount the ubifs which was created by the old NAND ecc layout.
> 
> This patch adds a new property : "fsl,use-mtd-ecc-info".

You have a regression in your driver. You need to fix the regression
first, so we can get that into 3.12 (or at this point, more likely
-stable). This could be something like you simple diff you sent earlier.

*After* we fix the regression, we can discuss new DT bindings.

> If we do not enable it, we will use the ecc strength calculated by ourselves
> and use all the OOB area (this is the legacy method);
> 
> If we enable it, we will use the ecc strength provided by the MTD layer,
> and create a new NAND ecc layout which may has free space in the OOB
> (for some SLC nand, we may support the JFFS2 with the new NAND ecc layout).
> 
> Signed-off-by: Huang Shijie <b32955-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  .../devicetree/bindings/mtd/gpmi-nand.txt          |    4 ++++
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c             |    7 ++++++-
>  2 files changed, 10 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/gpmi-nand.txt b/Documentation/devicetree/bindings/mtd/gpmi-nand.txt
> index 551b2a1..fe84a3d 100644
> --- a/Documentation/devicetree/bindings/mtd/gpmi-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/gpmi-nand.txt
> @@ -17,6 +17,10 @@ Required properties:
>  Optional properties:
>    - nand-on-flash-bbt: boolean to enable on flash bbt option if not
>                         present false
> +  - fsl,use-mtd-ecc-info: By enabling this boolean property, the gpmi can uses
> +                       the MTD's ECC info (if the MTD has). So we may have free
> +                       space in the OOB area, and we may support the JFFS2 with
> +                       some SLC nand, such as Micron's SLC nand.

As Mark mentioned, this description is very Linux-specific. And
honestly, I don't even understand what this description means except
that I reviewed your patches and this recent regression report. It is
not self explanatory at all.

Rhetorical question: What is "the MTD's ECC info"? What am I specifying
by putting this property in the device tree?

(Answer (?): you're looking for the ECC requirements from the data
sheet, which happens to be automatically discoverable in some cases.)

... so why don't you just add a DT property to actually specify the ECC
selection itself, rather than indirectly specifying it through a vague
specification and knowledge of how your driver handles it?

Or, is the difference between your "legacy" and "new" layout more than
just the choice of ECC strength and step size? If so, then this should
be documented [1], and that well-specified difference in layout should
become part of the DT binding.

>  The device tree may optionally contain sub-nodes describing partitions of the
>  address space. See partition.txt for more detail.
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index 5a1bbc7..62c0712 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -271,7 +271,9 @@ static int legacy_set_geometry(struct gpmi_nand_data *this)
>  		dev_err(this->dev,
>  			"We can not support this nand chip."
>  			" Its required ecc strength(%d) is beyond our"
> -			" capability(%d).\n", geo->ecc_strength,
> +			" capability(%d). If you have not enabled the"
> +			"'fsl,use-mtd-ecc-info', enable it and try again.\n",
> +			geo->ecc_strength,
>  			(GPMI_IS_MX6Q(this) ? MX6_ECC_STRENGTH_MAX
>  					: MXS_ECC_STRENGTH_MAX));
>  		return -EINVAL;
> @@ -352,6 +354,9 @@ static int legacy_set_geometry(struct gpmi_nand_data *this)
>  
>  int common_nfc_set_geometry(struct gpmi_nand_data *this)
>  {
> +	if (!of_property_read_bool(this->dev->of_node, "fsl,use-mtd-ecc-info"))
> +		return legacy_set_geometry(this);
> +
>  	return set_geometry_by_ecc_info(this) ? 0 : legacy_set_geometry(this);
>  }

Your first patch just needs to involve rearranging the calls to
legacy_set_geometry() and set_geometry_by_ecc_info() without any new
help from device tree.

Brian

[1] Is it somewhere in your 50-line comments in
drivers/mtd/nand/gpmi-nand/gpmi-nand.c? Perhaps this should migrate to
Documentation/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mtd: gpmi: fix the ecc regression

[Huang Shijie]

Hi David & Mark & Brian:

    thanks for your review and comments.
> ... so why don't you just add a DT property to actually specify the ECC
> selection itself, rather than indirectly specifying it through a vague
Do you mean i should add a DT property like "fsl, gpmi-use-new-ecclayout" ?

I am not understand "the ECC selection itself".



thanks
Huang Shijie



--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mtd: gpmi: fix the ecc regression

[Brian Norris]

On Thu, Oct 24, 2013 at 02:59:58PM +0800, Huang Shijie wrote:
> Hi David & Mark & Brian:
> 
>    thanks for your review and comments.
> >... so why don't you just add a DT property to actually specify the ECC
> >selection itself, rather than indirectly specifying it through a vague
> Do you mean i should add a DT property like "fsl, gpmi-use-new-ecclayout" ?
> 
> I am not understand "the ECC selection itself".

Sorry if that was unclear. I was recommending that the DT property
actually specify what the properties of the ECC layout were. Like ECC
strength (as an integer). Or spare area/step size/whatever else needed
to specify what these legacy and new layouts actually are. Your binding
was too vague; it wasn't describing what the layout is.

David seemed to suggest that something like your current binding might
be OK. (I still thing it was too vague, but if you can describe it
better, it may be sufficient.) A possible starting point:
"fsl,use-minimum-ecc". Does this properly reflect your intention of
using the datasheet's minimum required ECC (from ONFI or from the
driver's full ID table)?

But all that beside, I think your first priority is to fix the
regression for the current release cycle. I'm not keen on trying to
agree on a good DT binding and get it merged immediately.

Can you resend in two patches? The first one will default to using
legacy_set_geometry() to retain the old behavior and prevent
regressions. The second one can add a DT binding to re-introduce support
for your new layout. That way we can take the first one with no
question, and the second one can be taken when it's ready: either for
3.12 or 3.13.

Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mtd: gpmi: fix the ecc regression

[Huang Shijie]

于 2013年10月24日 15:52, Brian Norris 写道:
> David seemed to suggest that something like your current binding might
> be OK. (I still thing it was too vague, but if you can describe it
> better, it may be sufficient.) A possible starting point:
> "fsl,use-minimum-ecc". Does this properly reflect your intention of
> using the datasheet's minimum required ECC (from ONFI or from the
> driver's full ID table)?
yes, it's okay to me.

thanks
Huang Shijie

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] mtd: gpmi: fix the ecc regression

[Huang Shijie]

The patch "2febcdf mtd: gpmi: set the BCHs geometry with the ecc info"
makes the gpmi to use the ECC info provided by the MTD code, and creates
a different NAND ecc layout for the BCH which brings a regression to us:
   We can not mount the ubifs which was created by the old NAND ecc layout.

This patch adds a new property : "fsl,use-mtd-ecc-info".

If we do not enable it, we will use the ecc strength calculated by ourselves
and use all the OOB area (this is the legacy method);

If we enable it, we will use the ecc strength provided by the MTD layer,
and create a new NAND ecc layout which may has free space in the OOB
(for some SLC nand, we may support the JFFS2 with the new NAND ecc layout).

Signed-off-by: Huang Shijie <b32955-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 .../devicetree/bindings/mtd/gpmi-nand.txt          |    4 ++++
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c             |    7 ++++++-
 2 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/gpmi-nand.txt b/Documentation/devicetree/bindings/mtd/gpmi-nand.txt
index 551b2a1..fe84a3d 100644
--- a/Documentation/devicetree/bindings/mtd/gpmi-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/gpmi-nand.txt
@@ -17,6 +17,10 @@ Required properties:
 Optional properties:
   - nand-on-flash-bbt: boolean to enable on flash bbt option if not
                        present false
+  - fsl,use-mtd-ecc-info: By enabling this boolean property, the gpmi can uses
+                       the MTD's ECC info (if the MTD has). So we may have free
+                       space in the OOB area, and we may support the JFFS2 with
+                       some SLC nand, such as Micron's SLC nand.
 
 The device tree may optionally contain sub-nodes describing partitions of the
 address space. See partition.txt for more detail.
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 5a1bbc7..62c0712 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -271,7 +271,9 @@ static int legacy_set_geometry(struct gpmi_nand_data *this)
 		dev_err(this->dev,
 			"We can not support this nand chip."
 			" Its required ecc strength(%d) is beyond our"
-			" capability(%d).\n", geo->ecc_strength,
+			" capability(%d). If you have not enabled the"
+			"'fsl,use-mtd-ecc-info', enable it and try again.\n",
+			geo->ecc_strength,
 			(GPMI_IS_MX6Q(this) ? MX6_ECC_STRENGTH_MAX
 					: MXS_ECC_STRENGTH_MAX));
 		return -EINVAL;
@@ -352,6 +354,9 @@ static int legacy_set_geometry(struct gpmi_nand_data *this)
 
 int common_nfc_set_geometry(struct gpmi_nand_data *this)
 {
+	if (!of_property_read_bool(this->dev->of_node, "fsl,use-mtd-ecc-info"))
+		return legacy_set_geometry(this);
+
 	return set_geometry_by_ecc_info(this) ? 0 : legacy_set_geometry(this);
 }
 
-- 
1.7.2.rc3


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html