Re: [PATCH 1/5] mtd: nand: move layout structure into nand_ecc_ctrl

Re: [PATCH 1/5] mtd: nand: move layout structure into nand_ecc_ctrl

[David Brownell]

On Wednesday 13 May 2009, Troy Kisky wrote:
> +static const struct nand_ecclayout atmel_oobinfo_large __initdata = {

Doesn't this give you section warnings?  __initconst is what
I usually end up needing in such cases...

Also, for the davinci_nand driver this is going to need to
switch away from the ecclayout struct in the driver-private
area ... that's in a patch that (still) hasn't merged into
the MTD tree, but was stored there for the same reason that
you're submitting this patch:  the original code, using the
annoying ECC_HW_SYNDROME support, computed the layout.

- Dave

Re: [PATCH 2/5] mtd: nand: Calculate better default ecc layout

[David Brownell]

On Wednesday 13 May 2009, Troy Kisky wrote:
> This saves lines of code by having nand_base
> calculate oob_free entries.
> 
> Boards that don't specify a layout

Not specifying a layout ... that's fair, won't affect
compatibility with anything now deployed.  A nice new
feature, impacting nothing in-tree.

Another case would be anything using ECC_HW_SYNDROME,
where the layout is fully specified by ecc.prepad,
ecc.postpad, ecc.bytes, and the badblock marker
location ... and if any other layout is specified,
it will be ignored.  You could print warnings, and
make the ecclayout reported through the ioctls be
the actual layout derived from prepad/postpad/etc;
that counts as a pure bugfix.


> and don't 
> use all ecc bytes in the current default layout
> could have their ecc position changed.

That seems dangerous.  It will break all existing
boards.  You *really* don't want that on your head,
since it won't just break them but will probably
end up trashing all the data stored on their NAND
chips ...


> Looking through the code, I can only see that
> Davinci is effected this way. It has its ecc
> bytes moved to the end of the oob data, which
> is why I want this patch. Before, it was wasting
> a lot of oob bytes.

Erm, it took the default of 6 bytes ECC every 512
bytes of data ... which is more than the hardware
needs, just 3 bytes ECC for that much data.  That's
hardly a "lot" (3 extra ecc bytes per 512 data),
especially now that new software like UBI isn't
even using the oobdata areas.

Or the new 4-bit ECC code, *currently* supporting
only small page chips ... needs 10 bytes ECC for
each 512 bytes data.  There is no wasted space for
those cases.

You should be coordinating changes to davinci_nand
with Sneha, by the way.


> Some boards will print warning messages. i.e.
> mxc_nand which specifies ecc.bytes = 3, but
> eccbytes = 5 (not a multiple of 3).

I think it's fair to print a warning if an ECC layout
is provided and it's got nonfatal issues like that.

Or even print errors (and fail!) if something really
wrong turns up ... like mxc_nand getting a large-page
chip, so it needs eccbytes = 12 but gets an explicit
and undersized layout as you describe above.  (You
can see there's a pagesize_2k field that's never set.
There are clear bugs in large page support for the
mxc_nand driver, even if you ignore 4k pages.)


> I may have missed other boards being affected
> so it needs through testing.
> 
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>

Re: [PATCH 3/5] mtd: nand: atmel: use default ecc layout

[David Brownell]

On Wednesday 13 May 2009, Troy Kisky wrote:
> atmel should not need to override default

Doesn't this break compatibility with every system now
in the field?  As in, "install a new kernel, trash all
the data now in your NAND flash"??


> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> ---
>  drivers/mtd/nand/atmel_nand.c |   34 ----------------------------------
>  1 files changed, 0 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> index 7b52637..8537e86 100644
> --- a/drivers/mtd/nand/atmel_nand.c
> +++ b/drivers/mtd/nand/atmel_nand.c
> @@ -55,32 +55,6 @@
>  
>  #include "atmel_nand_ecc.h"	/* Hardware ECC registers */
>  
> -/* oob layout for large page size
> - * bad block info is on bytes 0 and 1
> - * the bytes have to be consecutives to avoid
> - * several NAND_CMD_RNDOUT during read
> - */
> -static const struct nand_ecclayout atmel_oobinfo_large __initdata = {
> -	.eccbytes = 4,
> -	.eccpos = {60, 61, 62, 63},
> -	.oobfree = {
> -		{2, 58}
> -	},
> -};
> -
> -/* oob layout for small page size
> - * bad block info is on bytes 4 and 5
> - * the bytes have to be consecutives to avoid
> - * several NAND_CMD_RNDOUT during read
> - */
> -static const struct nand_ecclayout atmel_oobinfo_small __initdata = {
> -	.eccbytes = 4,
> -	.eccpos = {0, 1, 2, 3},
> -	.oobfree = {
> -		{6, 10}
> -	},
> -};
> -
>  struct atmel_nand_host {
>  	struct nand_chip	nand_chip;
>  	struct mtd_info		mtd;
> @@ -477,23 +451,15 @@ static int __init atmel_nand_probe(struct platform_device *pdev)
>  		/* set ECC page size and oob layout */
>  		switch (mtd->writesize) {
>  		case 512:
> -			memcpy(&nand_chip->ecc.layout, &atmel_oobinfo_small,
> -					sizeof(nand_chip->ecc.layout));
>  			ecc_writel(host->ecc, MR, ATMEL_ECC_PAGESIZE_528);
>  			break;
>  		case 1024:
> -			memcpy(&nand_chip->ecc.layout, &atmel_oobinfo_large,
> -					sizeof(nand_chip->ecc.layout));
>  			ecc_writel(host->ecc, MR, ATMEL_ECC_PAGESIZE_1056);
>  			break;
>  		case 2048:
> -			memcpy(&nand_chip->ecc.layout, &atmel_oobinfo_large,
> -					sizeof(nand_chip->ecc.layout));
>  			ecc_writel(host->ecc, MR, ATMEL_ECC_PAGESIZE_2112);
>  			break;
>  		case 4096:
> -			memcpy(&nand_chip->ecc.layout, &atmel_oobinfo_large,
> -					sizeof(nand_chip->ecc.layout));
>  			ecc_writel(host->ecc, MR, ATMEL_ECC_PAGESIZE_4224);
>  			break;
>  		default:
> -- 
> 1.5.4.3
> 
> 

Re: [PATCH 4/5] mtd: nand: cafe_nand: use default ecc layout

[David Brownell]

On Wednesday 13 May 2009, Troy Kisky wrote:
> The bad block marker is at 14, but oobfree also
> starts at 14. This doesn't make sense to me,

On-chip bad-block tables stick the "Bbt0" (etc)
tags into the OOB data area, and don't need to
share that area with anything else.  It's OK.


> so I don't know if this patch is good or not.

Isn't this one of the drivers using NAND_ECC_HW_SYNDROME?
So that the ecclayout is fully defined by prepad==0?

This is a case where you *could* safely generate the
current ecclayout structs using simple rules.


> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> ---
>  drivers/mtd/nand/cafe_nand.c |   16 ----------------
>  1 files changed, 0 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/mtd/nand/cafe_nand.c b/drivers/mtd/nand/cafe_nand.c
> index c6c7f51..6048bd9 100644
> --- a/drivers/mtd/nand/cafe_nand.c
> +++ b/drivers/mtd/nand/cafe_nand.c
> @@ -456,12 +456,6 @@ static int cafe_nand_read_page(struct mtd_info *mtd, struct nand_chip *chip,
>  	return 0;
>  }
>  
> -static const struct nand_ecclayout cafe_oobinfo_2048 __devinitdata = {
> -	.eccbytes = 14,
> -	.eccpos = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13},
> -	.oobfree = {{14, 50}}
> -};
> -
>  /* Ick. The BBT code really ought to be able to work this bit out
>     for itself from the above, at least for the 2KiB case */
>  static uint8_t cafe_bbt_pattern_2048[] = { 'B', 'b', 't', '0' };
> @@ -491,12 +485,6 @@ static struct nand_bbt_descr cafe_bbt_mirror_descr_2048 = {
>  	.pattern = cafe_mirror_pattern_2048
>  };
>  
> -static const struct nand_ecclayout cafe_oobinfo_512 __devinitdata = {
> -	.eccbytes = 14,
> -	.eccpos = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13},
> -	.oobfree = {{14, 2}}
> -};
> -
>  static struct nand_bbt_descr cafe_bbt_main_descr_512 = {
>  	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
>  		| NAND_BBT_2BIT | NAND_BBT_VERSION,
> @@ -772,13 +760,9 @@ static int __devinit cafe_nand_probe(struct pci_dev *pdev,
>  
>  	/* Set up ECC according to the type of chip we found */
>  	if (mtd->writesize == 2048) {
> -		memcpy(&cafe->nand.ecc.layout, &cafe_oobinfo_2048,
> -				sizeof(cafe->nand.ecc.layout));
>  		cafe->nand.bbt_td = &cafe_bbt_main_descr_2048;
>  		cafe->nand.bbt_md = &cafe_bbt_mirror_descr_2048;
>  	} else if (mtd->writesize == 512) {
> -		memcpy(&cafe->nand.ecc.layout, &cafe_oobinfo_512,
> -				sizeof(cafe->nand.ecc.layout));
>  		cafe->nand.bbt_td = &cafe_bbt_main_descr_512;
>  		cafe->nand.bbt_md = &cafe_bbt_mirror_descr_512;
>  	} else {
> -- 
> 1.5.4.3
> 
> 

Re: [PATCH 3/5] mtd: nand: atmel: use default ecc layout

[Haavard Skinnemoen]

David Brownell wrote:
> On Wednesday 13 May 2009, Troy Kisky wrote:
> > atmel should not need to override default  
> 
> Doesn't this break compatibility with every system now
> in the field?  As in, "install a new kernel, trash all
> the data now in your NAND flash"??

I also suspect it will break hardware ECC. The ECC controller is a bit
picky about the OOB layout, which is why the defaults were overriden in
the first place.

Haavard

Re: [PATCH 1/5] mtd: nand: move layout structure into nand_ecc_ctrl

[Troy Kisky]

David Brownell wrote:
> On Wednesday 13 May 2009, Troy Kisky wrote:
>> +static const struct nand_ecclayout atmel_oobinfo_large __initdata = {
> 
> Doesn't this give you section warnings?  __initconst is what
> I usually end up needing in such cases...

Only if the same file has a non const __initdata variable.
Is __initconst the preferred method ?

> 
> Also, for the davinci_nand driver this is going to need to
> switch away from the ecclayout struct in the driver-private
> area ... that's in a patch that (still) hasn't merged into
> the MTD tree, but was stored there for the same reason that
> you're submitting this patch:  the original code, using the
> annoying ECC_HW_SYNDROME support, computed the layout.
> 
> - Dave

Is ECC_HW_SYNDROME only needed because the eccpos array is too small??

I'll have to review that prepad  and ECC_HW_SYNDROME
Thanks

Troy

Re: [PATCH 2/5] mtd: nand: Calculate better default ecc layout

[Troy Kisky]

David Brownell wrote:
> On Wednesday 13 May 2009, Troy Kisky wrote:
>> This saves lines of code by having nand_base
>> calculate oob_free entries.
>>
>> Boards that don't specify a layout
> 
> Not specifying a layout ... that's fair, won't affect
> compatibility with anything now deployed.  A nice new
> feature, impacting nothing in-tree.
> 
> Another case would be anything using ECC_HW_SYNDROME,
> where the layout is fully specified by ecc.prepad,
> ecc.postpad, ecc.bytes, and the badblock marker
> location ... and if any other layout is specified,
> it will be ignored.  You could print warnings, and
> make the ecclayout reported through the ioctls be
> the actual layout derived from prepad/postpad/etc;
> that counts as a pure bugfix.
> 
> 
>> and don't 
>> use all ecc bytes in the current default layout
>> could have their ecc position changed.
> 
> That seems dangerous.  It will break all existing
> boards.  You *really* don't want that on your head,
> since it won't just break them but will probably
> end up trashing all the data stored on their NAND
> chips ...
> 

That was an "and" above. Not specifying a specific
layout and using a default "and" not using all bytes
in the current default layouts ecc bytes.

> 
>> Looking through the code, I can only see that
>> Davinci is effected this way. It has its ecc
>> bytes moved to the end of the oob data, which
>> is why I want this patch. Before, it was wasting
>> a lot of oob bytes.
> 
> Erm, it took the default of 6 bytes ECC every 512
> bytes of data ... which is more than the hardware
> needs, just 3 bytes ECC for that much data.  That's
> hardly a "lot" (3 extra ecc bytes per 512 data),
> especially now that new software like UBI isn't
> even using the oobdata areas.
> 
> Or the new 4-bit ECC code, *currently* supporting
> only small page chips ... needs 10 bytes ECC for
> each 512 bytes data.  There is no wasted space for
> those cases.
> 
> You should be coordinating changes to davinci_nand
> with Sneha, by the way.

Whoever is in charge of the EVM will need to specify
a layout in the platform data if they don't like the change.

> 
> 
>> Some boards will print warning messages. i.e.
>> mxc_nand which specifies ecc.bytes = 3, but
>> eccbytes = 5 (not a multiple of 3).
> 
> I think it's fair to print a warning if an ECC layout
> is provided and it's got nonfatal issues like that.
> 
> Or even print errors (and fail!) if something really
> wrong turns up ... like mxc_nand getting a large-page
> chip, so it needs eccbytes = 12 but gets an explicit
> and undersized layout as you describe above.  (You
> can see there's a pagesize_2k field that's never set.
> There are clear bugs in large page support for the
> mxc_nand driver, even if you ignore 4k pages.)
> 
> 
>> I may have missed other boards being affected
>> so it needs through testing.
>>
>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> 
> 
> 

Re: [PATCH 3/5] mtd: nand: atmel: use default ecc layout

[Troy Kisky]

David Brownell wrote:
> On Wednesday 13 May 2009, Troy Kisky wrote:
>> atmel should not need to override default
> 
> Doesn't this break compatibility with every system now
> in the field?  As in, "install a new kernel, trash all
> the data now in your NAND flash"??
> 
> 
This patch relies on the 1st two patches. I should have said

atmel no longer needs to override the default. The layout specified
IS now the default layout for

nand_chip->ecc.bytes = 4

Troy

Re: [PATCH 1/5] mtd: nand: move layout structure into nand_ecc_ctrl

[David Brownell]

On Thursday 14 May 2009, Troy Kisky wrote:
> David Brownell wrote:
> > On Wednesday 13 May 2009, Troy Kisky wrote:
> >> +static const struct nand_ecclayout atmel_oobinfo_large __initdata = {
> > 
> > Doesn't this give you section warnings?  __initconst is what
> > I usually end up needing in such cases...
> 
> Only if the same file has a non const __initdata variable.
> Is __initconst the preferred method ?

Yes; it will always work, but __initdata won't always work.


> > Also, for the davinci_nand driver this is going to need to
> > switch away from the ecclayout struct in the driver-private
> > area ... that's in a patch that (still) hasn't merged into
> > the MTD tree, but was stored there for the same reason that
> > you're submitting this patch:  the original code, using the
> > annoying ECC_HW_SYNDROME support, computed the layout.
> > 
> > - Dave
> 
> Is ECC_HW_SYNDROME only needed because the eccpos array is too small??

No; it uses different algorithms to read/write, which
completely ignore ecclayout.  AFAICT ECC_HW_SYNDROME is
largely unused/untested except for single-chunk mode.

The size issue is that a 4-bit ECC code producing 10 bytes
of ECC per 512 bytes of data needs 80 bytes with 4K pages,
which obviously can't fit into the 64 bytes in ecclayout.

- Dave


> I'll have to review that prepad  and ECC_HW_SYNDROME
> Thanks
> 
> Troy
> 
> 

Re: [PATCH 3/5] mtd: nand: atmel: use default ecc layout

[Troy Kisky]

Haavard Skinnemoen wrote:
> David Brownell wrote:
>> On Wednesday 13 May 2009, Troy Kisky wrote:
>>> atmel should not need to override default  
>> Doesn't this break compatibility with every system now
>> in the field?  As in, "install a new kernel, trash all
>> the data now in your NAND flash"??
> 
> I also suspect it will break hardware ECC. The ECC controller is a bit
> picky about the OOB layout, which is why the defaults were overriden in
> the first place.
> 
> Haavard
> 
Thanks for responding.


This patch by itself would break ECCs. But with the 1st two patches
it shouldn't. I would love to have that assertion tested however.

Troy

Re: [PATCH 4/5] mtd: nand: cafe_nand: use default ecc layout

[Troy Kisky]

David Brownell wrote:
> On Wednesday 13 May 2009, Troy Kisky wrote:
>> The bad block marker is at 14, but oobfree also
>> starts at 14. This doesn't make sense to me,
> 
> On-chip bad-block tables stick the "Bbt0" (etc)
> tags into the OOB data area, and don't need to
> share that area with anything else.  It's OK.
> 
> 
>> so I don't know if this patch is good or not.
> 
> Isn't this one of the drivers using NAND_ECC_HW_SYNDROME?
> So that the ecclayout is fully defined by prepad==0?
> 
> This is a case where you *could* safely generate the
> current ecclayout structs using simple rules.
> 
> 

Actually, cafe_nand overrides most everything and doesn't use
prepad or postpad or ecc.layout. But I think it still relies
on ecc.layout.oobfree to setup chip->oob_poi. So, I'm no closer
to understanding why oobfree and bad block marker start at 14.

Troy

Re: [PATCH 3/5] mtd: nand: atmel: use default ecc layout

[Richard Genoud]

2009/5/14 Troy Kisky <troy.kisky@boundarydevices.com>:
> Haavard Skinnemoen wrote:
>> I also suspect it will break hardware ECC. The ECC controller is a bit
>> picky about the OOB layout, which is why the defaults were overriden in
>> the first place.
>>
>> Haavard
definitely.

>
> This patch by itself would break ECCs. But with the 1st two patches
> it shouldn't. I would love to have that assertion tested however.
>
> Troy
but where are those pathces ? I couldn't find them...

richard.

Re: [PATCH 3/5] mtd: nand: atmel: use default ecc layout

[Richard Genoud]

2009/5/15 Richard Genoud <richard.genoud@gmail.com>:
> 2009/5/14 Troy Kisky <troy.kisky@boundarydevices.com>:
>> Haavard Skinnemoen wrote:
>>> I also suspect it will break hardware ECC. The ECC controller is a bit
>>> picky about the OOB layout, which is why the defaults were overriden in
>>> the first place.
>>>
>>> Haavard

Looking at the first 2 patches on nand_base.c, it seems that the HW
ECC layout for small pages will be the same (ecc at offsets 0,1,2,3)
wich is good (because it's the only possible place).

For large pages, the HW ECC layout will change from [60,61,62,63] to
[2,3,4,5]. The ecc will still be correct, but as david said, the
compatibility won't.

richard.

Re: [PATCH 3/5] mtd: nand: atmel: use default ecc layout

[Troy Kisky]

Richard Genoud wrote:
> 2009/5/15 Richard Genoud <richard.genoud@gmail.com>:
>> 2009/5/14 Troy Kisky <troy.kisky@boundarydevices.com>:
>>> Haavard Skinnemoen wrote:
>>>> I also suspect it will break hardware ECC. The ECC controller is a bit
>>>> picky about the OOB layout, which is why the defaults were overriden in
>>>> the first place.
>>>>
>>>> Haavard
> 
> Looking at the first 2 patches on nand_base.c, it seems that the HW
> ECC layout for small pages will be the same (ecc at offsets 0,1,2,3)
> wich is good (because it's the only possible place).
> 
> For large pages, the HW ECC layout will change from [60,61,62,63] to
> [2,3,4,5]. The ecc will still be correct, but as david said, the
> compatibility won't.
> 
> richard.

I think you are wrong. It is definitely not what I intended.
The logic should say, "If I can place the ecc at the beginning I will,
otherwise, I will place it at the end." With the bad block marker
at 0, it should choose the end.


Troy

Re: [PATCH 3/5] mtd: nand: atmel: use default ecc layout

[Richard Genoud]

2009/5/16 Troy Kisky <troy.kisky@boundarydevices.com>:
> I think you are wrong. It is definitely not what I intended.
> The logic should say, "If I can place the ecc at the beginning I will,
> otherwise, I will place it at the end." With the bad block marker
> at 0, it should choose the end.
>
> Troy
>
You're right, I missed that.
So, for atmel_nand, nothing changes, and the hardware ecc code will
still be correct.
Maybe you could add a little comment in the code explaining the logic,
as you just did.

Acked-by: Richard Genoud <richard.genoud@gmail.com>

Re: [PATCH 3/5] mtd: nand: atmel: use default ecc layout

[Troy Kisky]

Richard Genoud wrote:
> 2009/5/16 Troy Kisky <troy.kisky@boundarydevices.com>:
>> I think you are wrong. It is definitely not what I intended.
>> The logic should say, "If I can place the ecc at the beginning I will,
>> otherwise, I will place it at the end." With the bad block marker
>> at 0, it should choose the end.
>>
>> Troy
>>
> You're right, I missed that.
> So, for atmel_nand, nothing changes, and the hardware ecc code will
> still be correct.
> Maybe you could add a little comment in the code explaining the logic,
> as you just did.
> 
> Acked-by: Richard Genoud <richard.genoud@gmail.com>

Ok, I will. Thanks for the review.

Troy

Re: [PATCH 3/5] mtd: nand: atmel: use default ecc layout

[Nicolas Ferre]

Hi,

I would like to bring my 2cents to the discussion so, let me comment simultaneously on:
[PATCH 1/5] mtd: nand: move layout structure into nand_ecc_ctrl
[PATCH 3/5] mtd: nand: atmel: use default ecc layout

First of all I did not manage to compile without this additional line in atmel_nand.c (which is not in original patch):

--- a/drivers/mtd/nand/atmel_nand.c
+++ b/drivers/mtd/nand/atmel_nand.c
@@ -329,7 +303,7 @@ static int atmel_nand_calculate(struct m
 {
 	struct nand_chip *nand_chip = mtd->priv;
 	struct atmel_nand_host *host = nand_chip->priv;
-	uint32_t *eccpos = nand_chip->ecc.layout->eccpos;
+	uint32_t *eccpos = nand_chip->ecc.layout.eccpos;
 	unsigned int ecc_value;
 
 	/* get the first 2 ECC bytes */


Here is the log that I have running with this patch:
"
NAND device: Manufacturer ID: 0xec, Chip ID: 0xda (Samsung NAND 256MiB 3,3V 8-bit)
AT91 NAND: 8-bit, Software ECC
nand_scan_tail ecc.total = 24, ecc.steps = 8, ecc.bytes = 3, ecc.size = 256, writesize = 2048
nand_scan_tail oobfree[0].offset=2, .length=38
Scanning device for bad blocks
Bad eraseblock 881 at 0x06e20000
Creating 3 MTD partitions on "atmel_nand":
0x00000000-0x00400000 : "Bootstrap"
0x00400000-0x04000000 : "Partition 1"
0x04000000-0x10000000 : "Partition 2"
"

All seems ok.

So, with the above line replaced, I would like to add to both patches:

[nicolas.ferre@atmel.com: tested on large page nand sw ECC]
Tested-by: Nicolas Ferre <nicolas.ferre@atmel.com>

Regards,
-- 
Nicolas Ferre

Re: [PATCH 3/5] mtd: nand: atmel: use default ecc layout

[Troy Kisky]

Nicolas Ferre wrote:
> Hi,
> 
> I would like to bring my 2cents to the discussion so, let me comment simultaneously on:
> [PATCH 1/5] mtd: nand: move layout structure into nand_ecc_ctrl
> [PATCH 3/5] mtd: nand: atmel: use default ecc layout
> 
> First of all I did not manage to compile without this additional line in atmel_nand.c (which is not in original patch):
> 
> --- a/drivers/mtd/nand/atmel_nand.c
> +++ b/drivers/mtd/nand/atmel_nand.c
> @@ -329,7 +303,7 @@ static int atmel_nand_calculate(struct m
>  {
>  	struct nand_chip *nand_chip = mtd->priv;
>  	struct atmel_nand_host *host = nand_chip->priv;
> -	uint32_t *eccpos = nand_chip->ecc.layout->eccpos;
> +	uint32_t *eccpos = nand_chip->ecc.layout.eccpos;
>  	unsigned int ecc_value;
>  
>  	/* get the first 2 ECC bytes */
> 

My patch set is on top of Linux 2.6.30-rc5.

>From 1/3
@@ -188,7 +188,6 @@ static int atmel_nand_calculate(struct mtd_info *mtd,
 {
 	struct nand_chip *nand_chip = mtd->priv;
 	struct atmel_nand_host *host = nand_chip->priv;
-	uint32_t *eccpos = nand_chip->ecc.layout->eccpos;
 	unsigned int ecc_value;

 	/* get the first 2 ECC bytes */

Which is the same subroutine as your change, but a strange difference in line
numbers. Which makes me think you have changes in this file as well.


> 
> Here is the log that I have running with this patch:
> "
> NAND device: Manufacturer ID: 0xec, Chip ID: 0xda (Samsung NAND 256MiB 3,3V 8-bit)
> AT91 NAND: 8-bit, Software ECC
> nand_scan_tail ecc.total = 24, ecc.steps = 8, ecc.bytes = 3, ecc.size = 256, writesize = 2048
> nand_scan_tail oobfree[0].offset=2, .length=38
> Scanning device for bad blocks
> Bad eraseblock 881 at 0x06e20000
> Creating 3 MTD partitions on "atmel_nand":
> 0x00000000-0x00400000 : "Bootstrap"
> 0x00400000-0x04000000 : "Partition 1"
> 0x04000000-0x10000000 : "Partition 2"
> "
> 
> All seems ok.
> 
> So, with the above line replaced, I would like to add to both patches:
> 
> [nicolas.ferre@atmel.com: tested on large page nand sw ECC]
> Tested-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> 
> Regards,


Thanks for testing. I appreciate it.


Troy