| Message ID | 1241663371-20448-1-git-send-email-nsnehaprabha@ti.com |
|---|---|
| State | New, archived |
| Headers | show |
I'm glad to see this patch is so small ... basically, just adding a special case for 2K pages, and keeping the core of this NAND driver the same. Not needing to change the 4-bit ECC support from the patch I sent earlier seems a good sign. Two comments though: (a) the board-dm355-evm.c file isn't yet in mainline, so the MTD folk can't take this patch as-is; (b) as noted elsewhere, there are still issues with 4K pages and the NAND core infrastructure. This patch is sufficient to support development boards for the dm355, dm357, and dm365 ... right? They all have 2 GByte NAND chips, ISTR with 2K pages, and haven't yet had to switch to more current parts with 4K pages. On Wednesday 06 May 2009, nsnehaprabha@ti.com wrote: > --- a/drivers/mtd/nand/davinci_nand.c > +++ b/drivers/mtd/nand/davinci_nand.c > @@ -500,6 +500,21 @@ static struct nand_ecclayout hwecc4_small __initconst > = { }, > }; > > +/* An ECC layout for using 4-bit ECC with large-page (2048bytes) flash, > + * storing ten ECC bytes plus the manufacturer's bad block marker byte, > + * and not overlapping the default BBT markers. > + */ > +static struct nand_ecclayout hwecc4_2048 __initconst = { > + .eccbytes = 10, Not ".eccbytes = 40"? This is 4 chunks, 10 ecc bytes each... > + .eccpos = { 0, 1, 2, 3, 4, 6, 7, 8, 9, 10, > + 11, 12, 13, 14, 15, 24, 25, 26, 27, 28, > + 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, > + 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, }, > + .oobfree = { > + {.offset = 16, .length = 8, }, > + {.offset = 49, }, Comments would be good, highlighting (a) byte 5 is reserved, it's the manufacturer bad block marker, (b) 8 bytes @16 are expected by JFFS2. Not everyone will "just know" those. > + }, > +}; > > static int __init nand_davinci_probe(struct platform_device *pdev) > { > @@ -690,6 +705,13 @@ static int __init nand_davinci_probe(struct platform_device *pdev) > info->mtd.oobsize - 16; > goto syndrome_done; > } > + if (chunks == 4) { > + info->ecclayout = hwecc4_2048; > + info->ecclayout.oobfree[1].length = > + info->mtd.oobsize - 49; > + info->chip.ecc.mode = NAND_ECC_HW_OOB_FIRST; > + goto syndrome_done; > + } > > /* For large page chips we'll be wanting to use a > * not-yet-implemented mode that reads OOB data You should update that comment ... that not-yet-implemented mode is now called "NAND_ECC_HW_OOB_FIRST", from patch 1/2 ... ;)
Dave, > -----Original Message----- > From: David Brownell [mailto:david-b@pacbell.net] > Sent: Thursday, May 07, 2009 3:17 AM > To: Narnakaje, Snehaprabha > Cc: davinci-linux-open-source@linux.davincidsp.com; linux- > mtd@lists.infradead.org; dwmw2@infradead.org; tglx@linutronix.de; > akpm@linux-foundation.org > Subject: Re: [PATCH 2/2] NAND on DM355: Add 4-bit ECC support for large > page NAND chips > > I'm glad to see this patch is so small ... basically, just > adding a special case for 2K pages, and keeping the core of > this NAND driver the same. Not needing to change the 4-bit > ECC support from the patch I sent earlier seems a good sign. Yes, I had to use the .calculate in the new read_page handler to stop the ECC though. > > Two comments though: (a) the board-dm355-evm.c file isn't > yet in mainline, so the MTD folk can't take this patch as-is; Sorry, I didn't realize this. I will separate the board-dm355-evm.c from this patch and submit the patch #3 for board-dm355-evm.c to go to davinci-linux-open-source. > (b) as noted elsewhere, there are still issues with 4K pages > and the NAND core infrastructure. Yes, this is still an issue, if we make the read_page handler use .eccpos[] for positioning the ECC bytes in the OOB area. If we had fixed prepad+ecc nand_ecclayout we would avoid using eccpos[] (This is what my initial set of 4-bit ECC patches did to support 4K). But this is not a generic solution. The main problem is with nand_ooblayout structure that is not extendable for large pages 4K or more, without breaking the userspace IOCTLs that is dependent on the size of this structure. Question to linux-mtd list: Are there plans in the linux-mtd to address this generic issue, now that NAND manufacturers are coming up NAND chips > 4K page size? > > This patch is sufficient to support development boards for > the dm355, dm357, and dm365 ... right? They all have 2 GByte > NAND chips, ISTR with 2K pages, and haven't yet had to switch > to more current parts with 4K pages. > > > On Wednesday 06 May 2009, nsnehaprabha@ti.com wrote: > > --- a/drivers/mtd/nand/davinci_nand.c > > +++ b/drivers/mtd/nand/davinci_nand.c > > @@ -500,6 +500,21 @@ static struct nand_ecclayout hwecc4_small > __initconst > > = { }, > > }; > > > > +/* An ECC layout for using 4-bit ECC with large-page (2048bytes) flash, > > + * storing ten ECC bytes plus the manufacturer's bad block marker byte, > > + * and not overlapping the default BBT markers. > > + */ > > +static struct nand_ecclayout hwecc4_2048 __initconst = { > > + .eccbytes = 10, > > Not ".eccbytes = 40"? This is 4 chunks, 10 ecc bytes each... No, .eccbytes is for each chips->ecc.steps. And all nand read/write APIs, we handle ecc.steps (for loop). There is chips->ecc.total that is initialized as (chips->ecc.steps * chips->ecc.bytes). It is strange that .eccbytes is for each chunk, while eccpos[] and .oobfree[] have to handle/cover all chunks. > > > > + .eccpos = { 0, 1, 2, 3, 4, 6, 7, 8, 9, 10, > > + 11, 12, 13, 14, 15, 24, 25, 26, 27, 28, > > + 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, > > + 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, }, > > + .oobfree = { > > + {.offset = 16, .length = 8, }, > > + {.offset = 49, }, > > Comments would be good, highlighting (a) byte 5 is reserved, > it's the manufacturer bad block marker, (b) 8 bytes @16 are > expected by JFFS2. Not everyone will "just know" those. OK, will add in the next patch. > > > > + }, > > +}; > > > > static int __init nand_davinci_probe(struct platform_device *pdev) > > { > > > @@ -690,6 +705,13 @@ static int __init nand_davinci_probe(struct > platform_device *pdev) > > info->mtd.oobsize - 16; > > goto syndrome_done; > > } > > + if (chunks == 4) { > > + info->ecclayout = hwecc4_2048; > > + info->ecclayout.oobfree[1].length = > > + info->mtd.oobsize - 49; > > + info->chip.ecc.mode = NAND_ECC_HW_OOB_FIRST; > > + goto syndrome_done; > > + } > > > > /* For large page chips we'll be wanting to use a > > * not-yet-implemented mode that reads OOB data > > You should update that comment ... that not-yet-implemented mode > is now called "NAND_ECC_HW_OOB_FIRST", from patch 1/2 ... ;) Yes, but will have a comment about 4K though. Thanks Sneha >
On Thursday 07 May 2009, Narnakaje, Snehaprabha wrote: > > I'm glad to see this patch is so small ... basically, just > > adding a special case for 2K pages, and keeping the core of > > this NAND driver the same. Not needing to change the 4-bit > > ECC support from the patch I sent earlier seems a good sign. > > Yes, I had to use the .calculate in the new read_page handler > to stop the ECC though. Oh, now I see what you were talking about off-list. That doesn't seem quite right. I'll take a look. > > Two comments though: (a) the board-dm355-evm.c file isn't > > yet in mainline, so the MTD folk can't take this patch as-is; > > Sorry, I didn't realize this. I will separate the board-dm355-evm.c > from this patch and submit the patch #3 for board-dm355-evm.c to go > to davinci-linux-open-source. Let's wait until we see the previous davinci_nand.c patches get queued up somewhere "official" though ... in fact, I don't see why they shouldn't merge for 2.6.30-soon. > > (b) as noted elsewhere, there are still issues with 4K pages > > and the NAND core infrastructure. > > Yes, this is still an issue, if we make the read_page handler > use .eccpos[] for positioning the ECC bytes in the OOB area. > If we had fixed prepad+ecc nand_ecclayout we would avoid using > eccpos[] (This is what my initial set of 4-bit ECC patches did > to support 4K). But this is not a generic solution. > > The main problem is with nand_ooblayout structure that is not > extendable for large pages 4K or more, without breaking the > userspace IOCTLs that is dependent on the size of this structure. > > Question to linux-mtd list: Are there plans in the linux-mtd to > address this generic issue, now that NAND manufacturers are > coming up NAND chips > 4K page size? They've been doing it for a while now, actually. :) And it wouldn't be an issue here ... *IF* this 4-bit ECC didn't need to use 80 bytes (8 chunks of 512 bytes, 10 bytes ECC per chunk) from a 64 byte field. Doesn't fit. Maybe the ecclayout stuff should migrate to sysfs, and just drop the old ioctl stuff for parts that can't use it ... as is being done to support chips that are 4 GByte and bigger. > > > +static struct nand_ecclayout hwecc4_2048 __initconst = { > > > + .eccbytes = 10, > > > > Not ".eccbytes = 40"? This is 4 chunks, 10 ecc bytes each... > > No, .eccbytes is for each chips->ecc.steps. So it's the same as ecc.bytes, which is not exported to userspace. > And all nand read/write > APIs, we handle ecc.steps (for loop). There is chips->ecc.total that > is initialized as (chips->ecc.steps * chips->ecc.bytes). > > It is strange that .eccbytes is for each chunk, while eccpos[] and > .oobfree[] have to handle/cover all chunks. Also strange: when ECC_HW_SYNDROME is in use, nothing even looks at the nand_ecclayout to make sure it matches the actual layout being used ... which derives entirely from the prepad, postpad, and bytes fields of "ecc". (Except that some of the OOB bytes not used for ECC may be hidden, e.g. manufacturer bad block markers are usually not listed.) - Dave
> -----Original Message----- > From: David Brownell [mailto:david-b@pacbell.net] > Sent: Thursday, May 07, 2009 1:11 PM > To: Narnakaje, Snehaprabha > Cc: davinci-linux-open-source@linux.davincidsp.com; linux- > mtd@lists.infradead.org; dwmw2@infradead.org; tglx@linutronix.de; > akpm@linux-foundation.org > Subject: Re: [PATCH 2/2] NAND on DM355: Add 4-bit ECC support for large > page NAND chips > > On Thursday 07 May 2009, Narnakaje, Snehaprabha wrote: > > > > I'm glad to see this patch is so small ... basically, just > > > adding a special case for 2K pages, and keeping the core of > > > this NAND driver the same. Not needing to change the 4-bit > > > ECC support from the patch I sent earlier seems a good sign. > > > > Yes, I had to use the .calculate in the new read_page handler > > to stop the ECC though. > > Oh, now I see what you were talking about off-list. That > doesn't seem quite right. I'll take a look. Looks like you had to implement is_readmode option in the .calculate function, to use the NAND_ECC_HW mode for small pages. The read_page handler for NAND_ECC_HW calls the .calculate function, and in our case we shouldn't be doing anything here for read. We use the .calculate function from write_page. > > > > > Two comments though: (a) the board-dm355-evm.c file isn't > > > yet in mainline, so the MTD folk can't take this patch as-is; > > > > Sorry, I didn't realize this. I will separate the board-dm355-evm.c > > from this patch and submit the patch #3 for board-dm355-evm.c to go > > to davinci-linux-open-source. > > Let's wait until we see the previous davinci_nand.c patches get > queued up somewhere "official" though ... in fact, I don't see > why they shouldn't merge for 2.6.30-soon. I was hoping my patch (dependent on your patch set) would at least trigger the review/acceptance :-) > > > > > (b) as noted elsewhere, there are still issues with 4K pages > > > and the NAND core infrastructure. > > > > Yes, this is still an issue, if we make the read_page handler > > use .eccpos[] for positioning the ECC bytes in the OOB area. > > If we had fixed prepad+ecc nand_ecclayout we would avoid using > > eccpos[] (This is what my initial set of 4-bit ECC patches did > > to support 4K). But this is not a generic solution. > > > > The main problem is with nand_ooblayout structure that is not > > extendable for large pages 4K or more, without breaking the > > userspace IOCTLs that is dependent on the size of this structure. > > > > Question to linux-mtd list: Are there plans in the linux-mtd to > > address this generic issue, now that NAND manufacturers are > > coming up NAND chips > 4K page size? > > They've been doing it for a while now, actually. :) > > And it wouldn't be an issue here ... *IF* this 4-bit ECC didn't > need to use 80 bytes (8 chunks of 512 bytes, 10 bytes ECC per > chunk) from a 64 byte field. Doesn't fit. > > Maybe the ecclayout stuff should migrate to sysfs, and just > drop the old ioctl stuff for parts that can't use it ... as > is being done to support chips that are 4 GByte and bigger. I will explore more on this option. > > > > > > +static struct nand_ecclayout hwecc4_2048 __initconst = { > > > > + .eccbytes = 10, > > > > > > Not ".eccbytes = 40"? This is 4 chunks, 10 ecc bytes each... > > > > No, .eccbytes is for each chips->ecc.steps. > > So it's the same as ecc.bytes, which is not exported to userspace. > > > > And all nand read/write > > APIs, we handle ecc.steps (for loop). There is chips->ecc.total that > > is initialized as (chips->ecc.steps * chips->ecc.bytes). > > > > It is strange that .eccbytes is for each chunk, while eccpos[] and > > .oobfree[] have to handle/cover all chunks. > > Also strange: when ECC_HW_SYNDROME is in use, nothing even > looks at the nand_ecclayout to make sure it matches the actual > layout being used ... which derives entirely from the prepad, > postpad, and bytes fields of "ecc". (Except that some of the > OOB bytes not used for ECC may be hidden, e.g. manufacturer > bad block markers are usually not listed.) If you leave enough prepad to cover manufacturer's BB and JFFS2 markers, this mechanism was OK, I guess. Thanks Sneha > > - Dave > > >
>>>> +static struct nand_ecclayout hwecc4_2048 __initconst = { >>>> + .eccbytes = 10, >>> Not ".eccbytes = 40"? This is 4 chunks, 10 ecc bytes each... >> No, .eccbytes is for each chips->ecc.steps. > > So it's the same as ecc.bytes, which is not exported to userspace. > > >> And all nand read/write >> APIs, we handle ecc.steps (for loop). There is chips->ecc.total that >> is initialized as (chips->ecc.steps * chips->ecc.bytes). >> >> It is strange that .eccbytes is for each chunk, while eccpos[] and >> .oobfree[] have to handle/cover all chunks. No, eccbytes should equal ecc.total i.e. from nand_base.c static struct nand_ecclayout nand_oob_64 = { .eccbytes = 24, .eccpos = { 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63}, .oobfree = { {.offset = 2, .length = 38}} }; Troy
> -----Original Message----- > From: Troy Kisky [mailto:troy.kisky@boundarydevices.com] > Sent: Thursday, May 07, 2009 6:37 PM > To: David Brownell > Cc: Narnakaje, Snehaprabha; dwmw2@infradead.org; davinci-linux-open- > source@linux.davincidsp.com; linux-mtd@lists.infradead.org; > tglx@linutronix.de; akpm@linux-foundation.org > Subject: Re: [PATCH 2/2] NAND on DM355: Add 4-bit ECC support for large > page NAND chips > > >>>> +static struct nand_ecclayout hwecc4_2048 __initconst = { > >>>> + .eccbytes = 10, > >>> Not ".eccbytes = 40"? This is 4 chunks, 10 ecc bytes each... > >> No, .eccbytes is for each chips->ecc.steps. > > > > So it's the same as ecc.bytes, which is not exported to userspace. > > > > > >> And all nand read/write > >> APIs, we handle ecc.steps (for loop). There is chips->ecc.total that > >> is initialized as (chips->ecc.steps * chips->ecc.bytes). > >> > >> It is strange that .eccbytes is for each chunk, while eccpos[] and > >> .oobfree[] have to handle/cover all chunks. > > No, eccbytes should equal ecc.total Thanks Troy and Dave. Yes, it should indeed be 40 --> total size. eccbytes in the nand_ecclayout is different from chip->ecc.bytes. We do set the chip->ecc.bytes to 10 which is for each trunk. I will change this and test it. > > > i.e. from nand_base.c > > static struct nand_ecclayout nand_oob_64 = { > .eccbytes = 24, > .eccpos = { > 40, 41, 42, 43, 44, 45, 46, 47, > 48, 49, 50, 51, 52, 53, 54, 55, > 56, 57, 58, 59, 60, 61, 62, 63}, > .oobfree = { > {.offset = 2, > .length = 38}} > }; > Troy > >
diff --git a/arch/arm/mach-davinci/board-dm355-evm.c b/arch/arm/mach-davinci/board-dm355-evm.c index f32e3d8..39c5a4e 100644 --- a/arch/arm/mach-davinci/board-dm355-evm.c +++ b/arch/arm/mach-davinci/board-dm355-evm.c @@ -85,8 +85,9 @@ static struct davinci_nand_pdata davinci_nand_data = { .mask_chipsel = BIT(14), .parts = davinci_nand_partitions, .nr_parts = ARRAY_SIZE(davinci_nand_partitions), - .ecc_mode = NAND_ECC_HW_SYNDROME, + .ecc_mode = NAND_ECC_HW, .options = NAND_USE_FLASH_BBT, + .ecc_bits = 4, }; static struct resource davinci_nand_resources[] = { diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c index 006843a..f84bc16 100644 --- a/drivers/mtd/nand/davinci_nand.c +++ b/drivers/mtd/nand/davinci_nand.c @@ -500,6 +500,21 @@ static struct nand_ecclayout hwecc4_small __initconst = { }, }; +/* An ECC layout for using 4-bit ECC with large-page (2048bytes) flash, + * storing ten ECC bytes plus the manufacturer's bad block marker byte, + * and not overlapping the default BBT markers. + */ +static struct nand_ecclayout hwecc4_2048 __initconst = { + .eccbytes = 10, + .eccpos = { 0, 1, 2, 3, 4, 6, 7, 8, 9, 10, + 11, 12, 13, 14, 15, 24, 25, 26, 27, 28, + 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, + 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, }, + .oobfree = { + {.offset = 16, .length = 8, }, + {.offset = 49, }, + }, +}; static int __init nand_davinci_probe(struct platform_device *pdev) { @@ -690,6 +705,13 @@ static int __init nand_davinci_probe(struct platform_device *pdev) info->mtd.oobsize - 16; goto syndrome_done; } + if (chunks == 4) { + info->ecclayout = hwecc4_2048; + info->ecclayout.oobfree[1].length = + info->mtd.oobsize - 49; + info->chip.ecc.mode = NAND_ECC_HW_OOB_FIRST; + goto syndrome_done; + } /* For large page chips we'll be wanting to use a * not-yet-implemented mode that reads OOB data