gpmi-mtd ecc regression

gpmi-mtd ecc regression

[Tim Harvey]

Huang,

The patch you made to obtain ECC info from the chip
(http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mtd/nand/gpmi-nand?id=2febcdf84b75aae627c61f0a5bf531a69299966c)
has caused a regression for an i.MX6 board I'm working with that uses
NAND and ubifs.

I'm using a Micron MT29F8G08
(http://download.micron.com/pdf/datasheets/flash/nand/2_4_8gb_nand_m49a.pdf)
which has:
page size: 2112 bytes (2048 + 64 bytes)
block size: 1056 words (1024 + 32 words)
plane size: 2 planes x 1024 blocks per plane
device size: 2Gb: 2048 blocks
ecc: 4bit

The legacy_set_geometry function comes up with:
gf_len=13
ecc_strength=8
page_size=2112
metadata_size=10
ecc_chunk_size=512
ecc_chunk_count=4
payload_size=2048
auxiliary_size=16
auxiliary_status_offset=12
block_mark_byte_offset=1999

and the new set_geometry_by_ecc_info comes up with:
gf_len=13
ecc_strength=4
page_size=2084
metadata_size=10
ecc_chunk_size=512
ecc_chunk_count=4
payload_size=2048
auxiliary_size=16
auxiliary_status_offset=12
block_mark_byte_offset=2018
block_mark_bit_offset=4

There are two discrepancies above:
 a) ecc_strength - this part has 4bit ecc but being detected as 8?
 b) page_size - the legacy function includes oob in page size, and the
new one does not

which is correct?

With your patch using the new function to detect ECC from the part,
booting to a ubifs that was flashed with uboot (which incidentally
also set ecc_strength=8) I get endless ECC failures:

[    3.946205] UBI: attaching mtd2 to ubi0
[    3.946585] UBI warning: ubi_io_read: error -74 (ECC error) while
reading 64 bytes from PEB 0:0, read only 64 bytes, retry
[    3.946785] UBI warning: ubi_io_read: error -74 (ECC error) while
reading 64 bytes from PEB 0:0, read only 64 bytes, retry
[    3.946983] UBI warning: ubi_io_read: error -74 (ECC error) while
reading 64 bytes from PEB 0:0, read only 64 bytes, retry
[    3.947177] UBI error: ubi_io_read: error -74 (ECC error) while
reading 64 bytes from PEB 0:0, read 64 bytes
[    3.947186] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.12.0-rc3+ #253
[    3.947192] Backtrace:
[    3.947220] [<8001265c>] (dump_backtrace+0x0/0x10c) from
[<800127fc>] (show_stack+0x18/0x1c)
[    3.947230]  r6:00000040 r5:ffffffb6 r4:00000000 r3:00000000
[    3.947247] [<800127e4>] (show_stack+0x0/0x1c) from [<80661820>]
(dump_stack+0x78/0x94)
[    3.947269] [<806617a8>] (dump_stack+0x0/0x94) from [<803b4ebc>]
(ubi_io_read+0x12c/0x364)
[    3.947274]  r4:bf09e000 r3:00000000
[    3.947285] [<803b4d90>] (ubi_io_read+0x0/0x364) from [<803b533c>]
(ubi_io_read_ec_hdr+0x60/0x330)
[    3.947295] [<803b52dc>] (ubi_io_read_ec_hdr+0x0/0x330) from
[<803bb2dc>] (ubi_attach+0x160/0x15f0)
[    3.947304] [<803bb17c>] (ubi_attach+0x0/0x15f0) from [<803ae5f0>]
(ubi_attach_mtd_dev+0x794/0xf18)
[    3.947319] [<803ade5c>] (ubi_attach_mtd_dev+0x0/0xf18) from
[<808d9850>] (ubi_init+0x1f8/0x2bc)
[    3.947330] [<808d9658>] (ubi_init+0x0/0x2bc) from [<80008880>]
(do_one_initcall+0xdc/0x194)
[    3.947342] [<800087a4>] (do_one_initcall+0x0/0x194) from
[<808aeb94>] (kernel_init_freeable+0x104/0x1d0)
[    3.952386] [<808d9658>] (ubi_init+0x0/0x2bc) from [<80008880>]
(do_one_initcall+0xdc/0x194)
[    3.960828] [<800087a4>] (do_one_initcall+0x0/0x194) from
[<808aeb94>] (kernel_init_freeable+0x104/0x1d0)
[    3.967418] [<800087a4>] (do_one_initcall+0x0/0x194) from
[<808aeb94>] (kernel_init_freeable+0x104/0x1d0)
[    3.967426] [<808aea90>] (kernel_init_freeable+0x0/0x1d0) from
[<8065c74c>] (kernel_init+0x10/0xec)

Ideas?

Thanks,

Tim

Re: gpmi-mtd ecc regression

[Tim Harvey]

On Fri, Oct 18, 2013 at 10:03 AM, Tim Harvey <tharvey@gateworks.com> wrote:
> Huang,
>
> The patch you made to obtain ECC info from the chip
> (http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mtd/nand/gpmi-nand?id=2febcdf84b75aae627c61f0a5bf531a69299966c)
> has caused a regression for an i.MX6 board I'm working with that uses
> NAND and ubifs.
>
> I'm using a Micron MT29F8G08
> (http://download.micron.com/pdf/datasheets/flash/nand/2_4_8gb_nand_m49a.pdf)
> which has:
> page size: 2112 bytes (2048 + 64 bytes)
> block size: 1056 words (1024 + 32 words)
> plane size: 2 planes x 1024 blocks per plane
> device size: 2Gb: 2048 blocks
> ecc: 4bit
>
> The legacy_set_geometry function comes up with:
> gf_len=13
> ecc_strength=8
> page_size=2112
> metadata_size=10
> ecc_chunk_size=512
> ecc_chunk_count=4
> payload_size=2048
> auxiliary_size=16
> auxiliary_status_offset=12
> block_mark_byte_offset=1999
>
> and the new set_geometry_by_ecc_info comes up with:
> gf_len=13
> ecc_strength=4

digging more into the issue I believe the problem is that the 'chip'
has an internal 4-bit ECC which is now being used, whereas what we
want is the BCH's ECC to be used (which would use 8bit ECC with a NAND
device with a pagesize of 2048 + 64 OOB).  If I force ecc_stength such
as:

@@ -143,7 +143,7 @@ static bool set_geometry_by_ecc_info(struct
gpmi_nand_data *this)
                return false;
        }
        geo->ecc_chunk_size = chip->ecc_step_ds;
-       geo->ecc_strength = round_up(chip->ecc_strength_ds, 2);
+       geo->ecc_strength = 8;
        if (!gpmi_check_ecc(this))
                return false;

@@ -208,6 +208,7 @@ static bool set_geometry_by_ecc_info(struct
gpmi_nand_data *this)

then I have no issues.

Does this make sense and if so should we be using the chip's reported
ecc strength at all?

Tim

Re: gpmi-mtd ecc regression

[Huang Shijie]

于 2013年10月19日 01:03, Tim Harvey 写道:
> Huang,
>
> The patch you made to obtain ECC info from the chip
> (http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mtd/nand/gpmi-nand?id=2febcdf84b75aae627c61f0a5bf531a69299966c)
> has caused a regression for an i.MX6 board I'm working with that uses
> NAND and ubifs.
>
> I'm using a Micron MT29F8G08
> (http://download.micron.com/pdf/datasheets/flash/nand/2_4_8gb_nand_m49a.pdf)
> which has:
> page size: 2112 bytes (2048 + 64 bytes)
> block size: 1056 words (1024 + 32 words)
> plane size: 2 planes x 1024 blocks per plane
> device size: 2Gb: 2048 blocks
> ecc: 4bit
>
> The legacy_set_geometry function comes up with:
> gf_len=13
> ecc_strength=8
> page_size=2112
> metadata_size=10
> ecc_chunk_size=512
> ecc_chunk_count=4
> payload_size=2048
> auxiliary_size=16
> auxiliary_status_offset=12
> block_mark_byte_offset=1999
>
> and the new set_geometry_by_ecc_info comes up with:
> gf_len=13
> ecc_strength=4
> page_size=2084
> metadata_size=10
> ecc_chunk_size=512
> ecc_chunk_count=4
> payload_size=2048
> auxiliary_size=16
> auxiliary_status_offset=12
> block_mark_byte_offset=2018
> block_mark_bit_offset=4
>
> There are two discrepancies above:
>   a) ecc_strength - this part has 4bit ecc but being detected as 8?
>   b) page_size - the legacy function includes oob in page size, and the
> new one does not
>
> which is correct?
>
In theory, both are correct.

I am in an embarrass state now:
[1] we can support the jffs2, when we use the set_geometry_by_ecc_info() 
to set the NAND layout.

[2] if we still use the legacy_set_geometry() to set the NAND layout, we 
can not support the jffs2 for GPMI.


When i submitted the gpmi to the kernel, the mtd code can not provide me 
the ECC info. I had to use all the OOB
with legacy_set_geometry(). After i submitted several patches for mtd, 
it can provide us the ECC info now, so i switch
to use the set_geometry_by_ecc_info().

it's easy to fix your problem with a patch like this:
-----------------------------------------------------------------------------------------------------------------

drivers/mtd/nand/gpmi-nand/gpmi-nand.c |    3 ++-
  1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand
index 5a1bbc7..8eeead6 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -352,7 +352,8 @@ static int legacy_set_geometry(struct gpmi_nand_data *this)

  int common_nfc_set_geometry(struct gpmi_nand_data *this)
  {
-       return set_geometry_by_ecc_info(this) ? 0 : legacy_set_geometry(this);
+       return (legacy_set_geometry(this) == 0) ? 0 :
+               set_geometry_by_ecc_info(this);
  }

  struct dma_chan *get_dma_chan(struct gpmi_nand_data *this)


-----------------------------------------------------------------------------------------------------------------

But after apply this patch, the gpmi may can not support the JFFS2 any more.

thanks
Huang Shijie



> With your patch using the new function to detect ECC from the part,
> booting to a ubifs that was flashed with uboot (which incidentally
> also set ecc_strength=8) I get endless ECC failures:
>
> [    3.946205] UBI: attaching mtd2 to ubi0
> [    3.946585] UBI warning: ubi_io_read: error -74 (ECC error) while
> reading 64 bytes from PEB 0:0, read only 64 bytes, retry
> [    3.946785] UBI warning: ubi_io_read: error -74 (ECC error) while
> reading 64 bytes from PEB 0:0, read only 64 bytes, retry
> [    3.946983] UBI warning: ubi_io_read: error -74 (ECC error) while
> reading 64 bytes from PEB 0:0, read only 64 bytes, retry
> [    3.947177] UBI error: ubi_io_read: error -74 (ECC error) while
> reading 64 bytes from PEB 0:0, read 64 bytes
> [    3.947186] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.12.0-rc3+ #253
> [    3.947192] Backtrace:
> [    3.947220] [<8001265c>] (dump_backtrace+0x0/0x10c) from
> [<800127fc>] (show_stack+0x18/0x1c)
> [    3.947230]  r6:00000040 r5:ffffffb6 r4:00000000 r3:00000000
> [    3.947247] [<800127e4>] (show_stack+0x0/0x1c) from [<80661820>]
> (dump_stack+0x78/0x94)
> [    3.947269] [<806617a8>] (dump_stack+0x0/0x94) from [<803b4ebc>]
> (ubi_io_read+0x12c/0x364)
> [    3.947274]  r4:bf09e000 r3:00000000
> [    3.947285] [<803b4d90>] (ubi_io_read+0x0/0x364) from [<803b533c>]
> (ubi_io_read_ec_hdr+0x60/0x330)
> [    3.947295] [<803b52dc>] (ubi_io_read_ec_hdr+0x0/0x330) from
> [<803bb2dc>] (ubi_attach+0x160/0x15f0)
> [    3.947304] [<803bb17c>] (ubi_attach+0x0/0x15f0) from [<803ae5f0>]
> (ubi_attach_mtd_dev+0x794/0xf18)
> [    3.947319] [<803ade5c>] (ubi_attach_mtd_dev+0x0/0xf18) from
> [<808d9850>] (ubi_init+0x1f8/0x2bc)
> [    3.947330] [<808d9658>] (ubi_init+0x0/0x2bc) from [<80008880>]
> (do_one_initcall+0xdc/0x194)
> [    3.947342] [<800087a4>] (do_one_initcall+0x0/0x194) from
> [<808aeb94>] (kernel_init_freeable+0x104/0x1d0)
> [    3.952386] [<808d9658>] (ubi_init+0x0/0x2bc) from [<80008880>]
> (do_one_initcall+0xdc/0x194)
> [    3.960828] [<800087a4>] (do_one_initcall+0x0/0x194) from
> [<808aeb94>] (kernel_init_freeable+0x104/0x1d0)
> [    3.967418] [<800087a4>] (do_one_initcall+0x0/0x194) from
> [<808aeb94>] (kernel_init_freeable+0x104/0x1d0)
> [    3.967426] [<808aea90>] (kernel_init_freeable+0x0/0x1d0) from
> [<8065c74c>] (kernel_init+0x10/0xec)
>
> Ideas?
>
> Thanks,
>
> Tim
>

Re: gpmi-mtd ecc regression

[Brian Norris]

On Sun, Oct 20, 2013 at 8:30 PM, Huang Shijie <b32955@freescale.com> wrote:
> 于 2013年10月19日 01:03, Tim Harvey 写道:
>>
>> The patch you made to obtain ECC info from the chip
>>
>> (http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mtd/nand/gpmi-nand?id=2febcdf84b75aae627c61f0a5bf531a69299966c)
>> has caused a regression for an i.MX6 board I'm working with that uses
>> NAND and ubifs.

...

> I am in an embarrass state now:
> [1] we can support the jffs2, when we use the set_geometry_by_ecc_info() to
> set the NAND layout.
>
> [2] if we still use the legacy_set_geometry() to set the NAND layout, we can
> not support the jffs2 for GPMI.
>
>
> When i submitted the gpmi to the kernel, the mtd code can not provide me the
> ECC info. I had to use all the OOB
> with legacy_set_geometry(). After i submitted several patches for mtd, it
> can provide us the ECC info now, so i switch
> to use the set_geometry_by_ecc_info().
>
> it's easy to fix your problem with a patch like this:
> -----------------------------------------------------------------------------------------------------------------
>
> drivers/mtd/nand/gpmi-nand/gpmi-nand.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> b/drivers/mtd/nand/gpmi-nand
> index 5a1bbc7..8eeead6 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -352,7 +352,8 @@ static int legacy_set_geometry(struct gpmi_nand_data
> *this)
>
>  int common_nfc_set_geometry(struct gpmi_nand_data *this)
>  {
> -       return set_geometry_by_ecc_info(this) ? 0 :
> legacy_set_geometry(this);
> +       return (legacy_set_geometry(this) == 0) ? 0 :
> +               set_geometry_by_ecc_info(this);
>  }
>
>  struct dma_chan *get_dma_chan(struct gpmi_nand_data *this)
>
>
> -----------------------------------------------------------------------------------------------------------------

So you really have two wholly incompatible layout methods, one which
targeted using the maximum ECC strength that fits and one which
targets the minimum required ECC strength? It seems that you can only
use a new ECC/geometry if the user prompts you to do so (via device
tree, for instance). Doing otherwise is bound to create regressions
like this.

> But after apply this patch, the gpmi may can not support the JFFS2 any more.

Well, we do prioritize "no regressions", rather than new features. So
we may have to do this until a proper solution can be formed.

Brian

Re: gpmi-mtd ecc regression

[Huang Shijie]

于 2013年10月21日 15:21, Brian Norris 写道:
> Well, we do prioritize "no regressions", rather than new features. So
> we may have to do this until a proper solution can be formed.
ok.

I will create a patch to fix this regression.
And it seems i have to add new devicetree property to enable the JFFS2
support.

thanks
Huang Shijie

Re: gpmi-mtd ecc regression

[Brian Norris]

On Mon, Oct 21, 2013 at 1:04 AM, Huang Shijie <b32955@freescale.com> wrote:
> 于 2013年10月21日 15:21, Brian Norris 写道:
>> Well, we do prioritize "no regressions", rather than new features. So
>> we may have to do this until a proper solution can be formed.
> ok.
>
> I will create a patch to fix this regression.

Yes. We should either get it into 3.12 or mark it -stable.

> And it seems i have to add new devicetree property to enable the JFFS2
> support.

We cannot use device-tree to enable JFFS2 (a purely software issue),
but you could use it to specify the ECC strength or layout. Be sure to
CC devicetree@vger.kernel.org on any new bindings, since they need to
review bindings and tend to have good input.

Brian

Re: gpmi-mtd ecc regression

[Marek Vasut]

Dear Huang Shijie,

> 于 2013年10月19日 01:03, Tim Harvey 写道:
> > Huang,
> > 
> > The patch you made to obtain ECC info from the chip
> > (http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/dr
> > ivers/mtd/nand/gpmi-nand?id=2febcdf84b75aae627c61f0a5bf531a69299966c) has
> > caused a regression for an i.MX6 board I'm working with that uses NAND
> > and ubifs.
> > 
> > I'm using a Micron MT29F8G08
> > (http://download.micron.com/pdf/datasheets/flash/nand/2_4_8gb_nand_m49a.p
> > df) which has:
> > page size: 2112 bytes (2048 + 64 bytes)
> > block size: 1056 words (1024 + 32 words)
> > plane size: 2 planes x 1024 blocks per plane
> > device size: 2Gb: 2048 blocks
> > ecc: 4bit
> > 
> > The legacy_set_geometry function comes up with:
> > gf_len=13
> > ecc_strength=8
> > page_size=2112
> > metadata_size=10
> > ecc_chunk_size=512
> > ecc_chunk_count=4
> > payload_size=2048
> > auxiliary_size=16
> > auxiliary_status_offset=12
> > block_mark_byte_offset=1999
> > 
> > and the new set_geometry_by_ecc_info comes up with:
> > gf_len=13
> > ecc_strength=4
> > page_size=2084
> > metadata_size=10
> > ecc_chunk_size=512
> > ecc_chunk_count=4
> > payload_size=2048
> > auxiliary_size=16
> > auxiliary_status_offset=12
> > block_mark_byte_offset=2018
> > block_mark_bit_offset=4
> > 
> > There are two discrepancies above:
> >   a) ecc_strength - this part has 4bit ecc but being detected as 8?
> >   b) page_size - the legacy function includes oob in page size, and the
> > 
> > new one does not
> > 
> > which is correct?
> 
> In theory, both are correct.
> 
> I am in an embarrass state now:
> [1] we can support the jffs2, when we use the set_geometry_by_ecc_info()
> to set the NAND layout.
> 
> [2] if we still use the legacy_set_geometry() to set the NAND layout, we
> can not support the jffs2 for GPMI.

It's exactly the other way around if you're talking about JFFS2 compatibility 
with the 2.6.35 FSL kernel ;-)