| Message ID | 1322973098-2528-3-git-send-email-shuo.liu@freescale.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Sun, 2011-12-04 at 12:31 +0800, shuo.liu@freescale.com wrote: > + /* > + * Freescale FCM controller has a 2K size limitation of buffer > + * RAM, so elbc_fcm_ctrl->buffer have to be used if writesize > + * of chip is greater than 2048. > + * We malloc a large enough buffer (maximum page size is 16K). > + */ > + elbc_fcm_ctrl->buffer = kmalloc(1024 * 16 + 1024, GFP_KERNEL); > + if (!elbc_fcm_ctrl->buffer) { > + dev_err(dev, "failed to allocate memory\n"); > + mutex_unlock(&fsl_elbc_nand_mutex); > + ret = -ENOMEM; > + goto err; > + } Sorry for returning to this again and agian - I do not have time to dig suggest you the right solutions on the one hand, you do not provide me a good answer on the other hand (or I forgot?). 16KiB pages do not even exist I believe. And you kmalloc 33KiB or RAM although in most cases you need only 5KiB. I think this is wrong - what is the very strong reason of wasting RAM you have? Why you cannot allocate exactly the required amount of RAM after 'nand_scan_ident()' finishes and you know the page size? Artem.
On 12/05/2011 12:47 AM, Artem Bityutskiy wrote: > On Sun, 2011-12-04 at 12:31 +0800, shuo.liu@freescale.com wrote: >> + /* >> + * Freescale FCM controller has a 2K size limitation of buffer >> + * RAM, so elbc_fcm_ctrl->buffer have to be used if writesize >> + * of chip is greater than 2048. >> + * We malloc a large enough buffer (maximum page size is 16K). >> + */ >> + elbc_fcm_ctrl->buffer = kmalloc(1024 * 16 + 1024, GFP_KERNEL); >> + if (!elbc_fcm_ctrl->buffer) { >> + dev_err(dev, "failed to allocate memory\n"); >> + mutex_unlock(&fsl_elbc_nand_mutex); >> + ret = -ENOMEM; >> + goto err; >> + } > > Sorry for returning to this again and agian - I do not have time to dig > suggest you the right solutions on the one hand, you do not provide me a > good answer on the other hand (or I forgot?). > > 16KiB pages do not even exist I believe. Googling turns up some hints of it, but nothing concrete such as a datasheet. We can assume 8K max for now and adjust it later, as the need becomes clear. > And you kmalloc 33KiB or RAM 17KiB, or 9KiB if we forget about 16K-page NAND. > although in most cases you need only 5KiB. I think this is wrong - > what is the very strong reason of wasting RAM you have? > > Why you cannot allocate exactly the required amount of RAM after > 'nand_scan_ident()' finishes and you know the page size? Because this is a controller resource, shared by multiple NAND chips that may be different page sizes (even if not, it's adding another point of synchronization required between initialization of different chips). I don't think it's worth the gymnastics to save a few KiB. -Scott
On Sun, 2011-12-04 at 12:31 +0800, shuo.liu@freescale.com wrote: > From: Liu Shuo <shuo.liu@freescale.com> > > Freescale FCM controller has a 2K size limitation of buffer RAM. In order > to support the Nand flash chip whose page size is larger than 2K bytes, > we read/write 2k data repeatedly by issuing FIR_OP_RB/FIR_OP_WB and save > them to a large buffer. > > Signed-off-by: Liu Shuo <shuo.liu@freescale.com> This patch does not apply on top of my l2-mtd-2.6.git tree. Could you please send a patch that applies cleanly? Thanks!
On Mon, 2011-12-05 at 13:46 -0600, Scott Wood wrote: > Because this is a controller resource, shared by multiple NAND chips > that may be different page sizes (even if not, it's adding another point > of synchronization required between initialization of different chips). > I don't think it's worth the gymnastics to save a few KiB. OK, I see. Artem.
On 12/03/2011 10:31 PM, shuo.liu@freescale.com wrote: > From: Liu Shuo <shuo.liu@freescale.com> > > Freescale FCM controller has a 2K size limitation of buffer RAM. In order > to support the Nand flash chip whose page size is larger than 2K bytes, > we read/write 2k data repeatedly by issuing FIR_OP_RB/FIR_OP_WB and save > them to a large buffer. > > Signed-off-by: Liu Shuo <shuo.liu@freescale.com> > --- > v3: > -remove page_size of struct fsl_elbc_mtd. > -do a oob write by NAND_CMD_RNDIN. > > drivers/mtd/nand/fsl_elbc_nand.c | 243 ++++++++++++++++++++++++++++++++++---- > 1 files changed, 218 insertions(+), 25 deletions(-) What is the plan for bad block marker migration? > @@ -473,13 +568,72 @@ static void fsl_elbc_cmdfunc(struct mtd_info *mtd, unsigned int command, > * write so the HW generates the ECC. > */ > if (elbc_fcm_ctrl->oob || elbc_fcm_ctrl->column != 0 || > - elbc_fcm_ctrl->index != mtd->writesize + mtd->oobsize) > - out_be32(&lbc->fbcr, > - elbc_fcm_ctrl->index - elbc_fcm_ctrl->column); > - else > + elbc_fcm_ctrl->index != mtd->writesize + mtd->oobsize) { > + if (elbc_fcm_ctrl->oob && mtd->writesize > 2048) { > + out_be32(&lbc->fbcr, 64); > + } else { > + out_be32(&lbc->fbcr, elbc_fcm_ctrl->index > + - elbc_fcm_ctrl->column); > + } We need to limit ourselves to the regions that have actually been written to in the buffer. fbcr needs to be set separately for first and last subpages, with intermediate subpages having 0, 64, or 2112 as appropriate. Subpages that are entirely before column or entirely after column + index should be skipped. > + } else { > + out_be32(&lbc->fir, FIR_OP_WB << FIR_OP1_SHIFT); > + for (i = 1; i < n; i++) { > + if (i == n - 1) { > + elbc_fcm_ctrl->use_mdr = 1; > + out_be32(&lbc->fir, > + (FIR_OP_WB << FIR_OP1_SHIFT) | > + (FIR_OP_CM3 << FIR_OP2_SHIFT) | > + (FIR_OP_CW1 << FIR_OP3_SHIFT) | > + (FIR_OP_RS << FIR_OP4_SHIFT)); Please explicitly show the (FIR_OP_NOP << FIR_OP0_SHIFT) compenent. > + } else if (mtd->writesize >= 2048 && mtd->writesize <= 16 * 1024) { > + > setbits32(&lbc->bank[priv->bank].or, OR_FCM_PGS); Don't insert a blank line here. -Scott
于 2011年12月07日 08:09, Scott Wood 写道: > On 12/03/2011 10:31 PM, shuo.liu@freescale.com wrote: >> From: Liu Shuo<shuo.liu@freescale.com> >> >> Freescale FCM controller has a 2K size limitation of buffer RAM. In order >> to support the Nand flash chip whose page size is larger than 2K bytes, >> we read/write 2k data repeatedly by issuing FIR_OP_RB/FIR_OP_WB and save >> them to a large buffer. >> >> Signed-off-by: Liu Shuo<shuo.liu@freescale.com> >> --- >> v3: >> -remove page_size of struct fsl_elbc_mtd. >> -do a oob write by NAND_CMD_RNDIN. >> >> drivers/mtd/nand/fsl_elbc_nand.c | 243 ++++++++++++++++++++++++++++++++++---- >> 1 files changed, 218 insertions(+), 25 deletions(-) > What is the plan for bad block marker migration? This patch has been ported to uboot now, I think we can make a special uboot image for bad block marker migration when first use the chip. >> @@ -473,13 +568,72 @@ static void fsl_elbc_cmdfunc(struct mtd_info *mtd, unsigned int command, >> * write so the HW generates the ECC. >> */ >> if (elbc_fcm_ctrl->oob || elbc_fcm_ctrl->column != 0 || >> - elbc_fcm_ctrl->index != mtd->writesize + mtd->oobsize) >> - out_be32(&lbc->fbcr, >> - elbc_fcm_ctrl->index - elbc_fcm_ctrl->column); >> - else >> + elbc_fcm_ctrl->index != mtd->writesize + mtd->oobsize) { >> + if (elbc_fcm_ctrl->oob&& mtd->writesize> 2048) { >> + out_be32(&lbc->fbcr, 64); >> + } else { >> + out_be32(&lbc->fbcr, elbc_fcm_ctrl->index >> + - elbc_fcm_ctrl->column); >> + } > We need to limit ourselves to the regions that have actually been > written to in the buffer. fbcr needs to be set separately for first and > last subpages, with intermediate subpages having 0, 64, or 2112 as > appropriate. Subpages that are entirely before column or entirely after > column + index should be skipped. I have considered this case, but I don't think it is useful. 1.There isn't a 'length' parameter in driver interface, although we can get it from 'index - column'. 2.To see nand_do_write_oob() in nand_base.c, it fill '0xff' to entire oob area first and write the user data by nand_fill_oob(), then call ecc.write_oob (default is nand_write_oob_std()). 'column' is mtd->writesize and 'length' of write_buf() is mtd->oobsize. So I don't think we need to deal with it there. >> + } else { >> + out_be32(&lbc->fir, FIR_OP_WB<< FIR_OP1_SHIFT); >> + for (i = 1; i< n; i++) { >> + if (i == n - 1) { >> + elbc_fcm_ctrl->use_mdr = 1; >> + out_be32(&lbc->fir, >> + (FIR_OP_WB<< FIR_OP1_SHIFT) | >> + (FIR_OP_CM3<< FIR_OP2_SHIFT) | >> + (FIR_OP_CW1<< FIR_OP3_SHIFT) | >> + (FIR_OP_RS<< FIR_OP4_SHIFT)); > Please explicitly show the (FIR_OP_NOP<< FIR_OP0_SHIFT) compenent. > >> + } else if (mtd->writesize>= 2048&& mtd->writesize<= 16 * 1024) { >> + >> setbits32(&lbc->bank[priv->bank].or, OR_FCM_PGS); > Don't insert a blank line here. > Ok. -LiuShuo > -Scott
On 12/06/2011 09:55 PM, LiuShuo wrote: > 于 2011年12月07日 08:09, Scott Wood 写道: >> On 12/03/2011 10:31 PM, shuo.liu@freescale.com wrote: >>> From: Liu Shuo<shuo.liu@freescale.com> >>> >>> Freescale FCM controller has a 2K size limitation of buffer RAM. In >>> order >>> to support the Nand flash chip whose page size is larger than 2K bytes, >>> we read/write 2k data repeatedly by issuing FIR_OP_RB/FIR_OP_WB and save >>> them to a large buffer. >>> >>> Signed-off-by: Liu Shuo<shuo.liu@freescale.com> >>> --- >>> v3: >>> -remove page_size of struct fsl_elbc_mtd. >>> -do a oob write by NAND_CMD_RNDIN. >>> >>> drivers/mtd/nand/fsl_elbc_nand.c | 243 >>> ++++++++++++++++++++++++++++++++++---- >>> 1 files changed, 218 insertions(+), 25 deletions(-) >> What is the plan for bad block marker migration? > This patch has been ported to uboot now, I think we can make a special > uboot image for bad > block marker migration when first use the chip. It should not be a special image, and there should be some way to mark that the migration has happened. Even if we do the migration in U-Boot, Linux could check for the marker and if absent, disallow access and tell the user to run the migration tool. >>> @@ -473,13 +568,72 @@ static void fsl_elbc_cmdfunc(struct mtd_info >>> *mtd, unsigned int command, >>> * write so the HW generates the ECC. >>> */ >>> if (elbc_fcm_ctrl->oob || elbc_fcm_ctrl->column != 0 || >>> - elbc_fcm_ctrl->index != mtd->writesize + mtd->oobsize) >>> - out_be32(&lbc->fbcr, >>> - elbc_fcm_ctrl->index - elbc_fcm_ctrl->column); >>> - else >>> + elbc_fcm_ctrl->index != mtd->writesize + mtd->oobsize) { >>> + if (elbc_fcm_ctrl->oob&& mtd->writesize> 2048) { >>> + out_be32(&lbc->fbcr, 64); >>> + } else { >>> + out_be32(&lbc->fbcr, elbc_fcm_ctrl->index >>> + - elbc_fcm_ctrl->column); >>> + } >> We need to limit ourselves to the regions that have actually been >> written to in the buffer. fbcr needs to be set separately for first and >> last subpages, with intermediate subpages having 0, 64, or 2112 as >> appropriate. Subpages that are entirely before column or entirely after >> column + index should be skipped. > > I have considered this case, but I don't think it is useful. > 1.There isn't a 'length' parameter in driver interface, although we > can get it from 'index - column'. Right. column is start, and index is end + 1. We have the bounds of what has been written. > 2.To see nand_do_write_oob() in nand_base.c, it fill '0xff' to > entire oob area first and write the user data by nand_fill_oob(), then > call ecc.write_oob (default is nand_write_oob_std()). Do we really want to assume that that's what it will always do? And if we do want to make such assumptions, we could rip out all usage of index/column here, and just handle "oob" and "full page" cases. -Scott
于 2011年12月08日 03:11, Scott Wood 写道: > On 12/06/2011 09:55 PM, LiuShuo wrote: >> 于 2011年12月07日 08:09, Scott Wood 写道: >>> On 12/03/2011 10:31 PM, shuo.liu@freescale.com wrote: >>>> From: Liu Shuo<shuo.liu@freescale.com> >>>> >>>> Freescale FCM controller has a 2K size limitation of buffer RAM. In >>>> order >>>> to support the Nand flash chip whose page size is larger than 2K bytes, >>>> we read/write 2k data repeatedly by issuing FIR_OP_RB/FIR_OP_WB and save >>>> them to a large buffer. >>>> >>>> Signed-off-by: Liu Shuo<shuo.liu@freescale.com> >>>> --- >>>> v3: >>>> -remove page_size of struct fsl_elbc_mtd. >>>> -do a oob write by NAND_CMD_RNDIN. >>>> >>>> drivers/mtd/nand/fsl_elbc_nand.c | 243 >>>> ++++++++++++++++++++++++++++++++++---- >>>> 1 files changed, 218 insertions(+), 25 deletions(-) >>> What is the plan for bad block marker migration? >> This patch has been ported to uboot now, I think we can make a special >> uboot image for bad >> block marker migration when first use the chip. > It should not be a special image, and there should be some way to mark > that the migration has happened. Even if we do the migration in U-Boot, > Linux could check for the marker and if absent, disallow access and tell > the user to run the migration tool. > >>>> @@ -473,13 +568,72 @@ static void fsl_elbc_cmdfunc(struct mtd_info >>>> *mtd, unsigned int command, >>>> * write so the HW generates the ECC. >>>> */ >>>> if (elbc_fcm_ctrl->oob || elbc_fcm_ctrl->column != 0 || >>>> - elbc_fcm_ctrl->index != mtd->writesize + mtd->oobsize) >>>> - out_be32(&lbc->fbcr, >>>> - elbc_fcm_ctrl->index - elbc_fcm_ctrl->column); >>>> - else >>>> + elbc_fcm_ctrl->index != mtd->writesize + mtd->oobsize) { >>>> + if (elbc_fcm_ctrl->oob&& mtd->writesize> 2048) { >>>> + out_be32(&lbc->fbcr, 64); >>>> + } else { >>>> + out_be32(&lbc->fbcr, elbc_fcm_ctrl->index >>>> + - elbc_fcm_ctrl->column); >>>> + } >>> We need to limit ourselves to the regions that have actually been >>> written to in the buffer. fbcr needs to be set separately for first and >>> last subpages, with intermediate subpages having 0, 64, or 2112 as >>> appropriate. Subpages that are entirely before column or entirely after >>> column + index should be skipped. >> I have considered this case, but I don't think it is useful. >> 1.There isn't a 'length' parameter in driver interface, although we >> can get it from 'index - column'. > Right. column is start, and index is end + 1. We have the bounds of > what has been written. > >> 2.To see nand_do_write_oob() in nand_base.c, it fill '0xff' to >> entire oob area first and write the user data by nand_fill_oob(), then >> call ecc.write_oob (default is nand_write_oob_std()). > Do we really want to assume that that's what it will always do? > > And if we do want to make such assumptions, we could rip out all usage > of index/column here, and just handle "oob" and "full page" cases. The function nand_do_write_ops() in nandbase.c is a Nand internal interface. It always is called when application write to nand flash. (e.g. dd) In this function, partial page write is dealt with by filling '0xff' to buffer before data copy. (nand_do_write_oob() is similar) So I don't think we need to do it in our controller driver again, it should be a job of upper layer. I found that 'column' for NAND_CMD_SEQIN is always 0 or writesize except for oob write with NAND_ECC_HW_SYNDROME, but it's not useful case for our controller. -LiuShuo > -Scott
On 12/08/2011 04:44 AM, LiuShuo wrote: > 于 2011年12月08日 03:11, Scott Wood 写道: >> And if we do want to make such assumptions, we could rip out all usage >> of index/column here, and just handle "oob" and "full page" cases. > The function nand_do_write_ops() in nandbase.c is a Nand internal > interface. > It always is called when application write to nand flash. (e.g. dd) > In this function, partial page write is dealt with by filling '0xff' to > buffer before data copy. > (nand_do_write_oob() is similar) > So I don't think we need to do it in our controller driver again, it > should be a job of upper layer. If this is to be considered part of the interface contract, then perhaps do a WARN_ON() if we see an unexpected index/column? And after that, only consider full-page or full-oob possibilities. -Scott
On Tue, 2011-12-06 at 18:09 -0600, Scott Wood wrote: > On 12/03/2011 10:31 PM, shuo.liu@freescale.com wrote: > > From: Liu Shuo <shuo.liu@freescale.com> > > > > Freescale FCM controller has a 2K size limitation of buffer RAM. In order > > to support the Nand flash chip whose page size is larger than 2K bytes, > > we read/write 2k data repeatedly by issuing FIR_OP_RB/FIR_OP_WB and save > > them to a large buffer. > > > > Signed-off-by: Liu Shuo <shuo.liu@freescale.com> > > --- > > v3: > > -remove page_size of struct fsl_elbc_mtd. > > -do a oob write by NAND_CMD_RNDIN. > > > > drivers/mtd/nand/fsl_elbc_nand.c | 243 ++++++++++++++++++++++++++++++++++---- > > 1 files changed, 218 insertions(+), 25 deletions(-) > > What is the plan for bad block marker migration? Why it should be migrated? I thought that you support 2KiB pages, and this adds 4 and 8 KiB pages support, which you never supported before. What is the migration you guys are talking about? Artem.
On 12/12/2011 03:09 PM, Artem Bityutskiy wrote: > On Tue, 2011-12-06 at 18:09 -0600, Scott Wood wrote: >> On 12/03/2011 10:31 PM, shuo.liu@freescale.com wrote: >>> From: Liu Shuo <shuo.liu@freescale.com> >>> >>> Freescale FCM controller has a 2K size limitation of buffer RAM. In order >>> to support the Nand flash chip whose page size is larger than 2K bytes, >>> we read/write 2k data repeatedly by issuing FIR_OP_RB/FIR_OP_WB and save >>> them to a large buffer. >>> >>> Signed-off-by: Liu Shuo <shuo.liu@freescale.com> >>> --- >>> v3: >>> -remove page_size of struct fsl_elbc_mtd. >>> -do a oob write by NAND_CMD_RNDIN. >>> >>> drivers/mtd/nand/fsl_elbc_nand.c | 243 ++++++++++++++++++++++++++++++++++---- >>> 1 files changed, 218 insertions(+), 25 deletions(-) >> >> What is the plan for bad block marker migration? > > Why it should be migrated? I thought that you support 2KiB pages, and > this adds 4 and 8 KiB pages support, which you never supported before. > What is the migration you guys are talking about? NAND chips come from the factory with bad blocks marked at a certain offset into each page. This offset is normally in the OOB area, but since we change the layout from "4k data, 128 byte oob" to "2k data, 64 byte oob, 2k data, 64 byte oob" the marker is no longer in the oob. On first use we need to migrate the markers so that they are still in the oob. -Scott
On Mon, 2011-12-12 at 15:15 -0600, Scott Wood wrote: > NAND chips come from the factory with bad blocks marked at a certain > offset into each page. This offset is normally in the OOB area, but > since we change the layout from "4k data, 128 byte oob" to "2k data, 64 > byte oob, 2k data, 64 byte oob" the marker is no longer in the oob. On > first use we need to migrate the markers so that they are still in the oob. Ah, I see, thanks. Are you planning to implement in-kernel migration or use a user-space tool? Artem.
On 12/12/2011 03:19 PM, Artem Bityutskiy wrote: > On Mon, 2011-12-12 at 15:15 -0600, Scott Wood wrote: >> NAND chips come from the factory with bad blocks marked at a certain >> offset into each page. This offset is normally in the OOB area, but >> since we change the layout from "4k data, 128 byte oob" to "2k data, 64 >> byte oob, 2k data, 64 byte oob" the marker is no longer in the oob. On >> first use we need to migrate the markers so that they are still in the oob. > > Ah, I see, thanks. Are you planning to implement in-kernel migration or > use a user-space tool? That's the kind of answer I was hoping to get from Shuo. :-) Most likely is a firmware-based tool, but I'd like there to be some way for the tool to mark that this has happened, so that the Linux driver can refuse to do non-raw accesses to a chip that isn't marked as having been migrated (or at least yell loudly in the log). Speaking of raw accesses, these are currently broken in the eLBC driver... we need some way for the generic layer to tell us what kind of access it is before the transaction starts, not once it wants to read out the buffer (unless we add more hacks to delay the start of a read transaction until first buffer access...). We'd be better off with a high-level "read page/write page" function that does the whole thing (not just buffer access, but command issuance as well). -Scott
于 2011年12月13日 05:30, Scott Wood 写道: > On 12/12/2011 03:19 PM, Artem Bityutskiy wrote: >> On Mon, 2011-12-12 at 15:15 -0600, Scott Wood wrote: >>> NAND chips come from the factory with bad blocks marked at a certain >>> offset into each page. This offset is normally in the OOB area, but >>> since we change the layout from "4k data, 128 byte oob" to "2k data, 64 >>> byte oob, 2k data, 64 byte oob" the marker is no longer in the oob. On >>> first use we need to migrate the markers so that they are still in the oob. >> Ah, I see, thanks. Are you planning to implement in-kernel migration or >> use a user-space tool? > That's the kind of answer I was hoping to get from Shuo. :-) OK, I try to do this. Wait for a couple of days. -LiuShuo > Most likely is a firmware-based tool, but I'd like there to be some way > for the tool to mark that this has happened, so that the Linux driver > can refuse to do non-raw accesses to a chip that isn't marked as having > been migrated (or at least yell loudly in the log). > > Speaking of raw accesses, these are currently broken in the eLBC > driver... we need some way for the generic layer to tell us what kind of > access it is before the transaction starts, not once it wants to read > out the buffer (unless we add more hacks to delay the start of a read > transaction until first buffer access...). We'd be better off with a > high-level "read page/write page" function that does the whole thing > (not just buffer access, but command issuance as well). > > -Scott
于 2011年12月13日 05:09, Artem Bityutskiy 写道: > On Tue, 2011-12-06 at 18:09 -0600, Scott Wood wrote: >> On 12/03/2011 10:31 PM, shuo.liu@freescale.com wrote: >>> From: Liu Shuo<shuo.liu@freescale.com> >>> >>> Freescale FCM controller has a 2K size limitation of buffer RAM. In order >>> to support the Nand flash chip whose page size is larger than 2K bytes, >>> we read/write 2k data repeatedly by issuing FIR_OP_RB/FIR_OP_WB and save >>> them to a large buffer. >>> >>> Signed-off-by: Liu Shuo<shuo.liu@freescale.com> >>> --- >>> v3: >>> -remove page_size of struct fsl_elbc_mtd. >>> -do a oob write by NAND_CMD_RNDIN. >>> >>> drivers/mtd/nand/fsl_elbc_nand.c | 243 ++++++++++++++++++++++++++++++++++---- >>> 1 files changed, 218 insertions(+), 25 deletions(-) >> What is the plan for bad block marker migration. I think we can use a special bbt pattern to indicate whether migration has been done. (we needn't to define another marker) Do the migration our chip->scan_bbt as follow : /* * this pattern indicate that the bad block information has been migrated, * if this isn't found, we do the migration. */ static u8 migrated_bbt_pattern[] = {'M', 'b', 'b', 't', '0' }; static int fsl_elbc_bbt(struct mtd_info *mtd) { if (!check_migrated_bbt_pattern()) bad_block_info_migtrate(); nand_default_bbt(mtd); /* default function in nand_bbt.c */ } - LiuShuo
于 2011年12月13日 10:46, LiuShuo 写道: > 于 2011年12月13日 05:30, Scott Wood 写道: >> On 12/12/2011 03:19 PM, Artem Bityutskiy wrote: >>> On Mon, 2011-12-12 at 15:15 -0600, Scott Wood wrote: >>>> NAND chips come from the factory with bad blocks marked at a certain >>>> offset into each page. This offset is normally in the OOB area, but >>>> since we change the layout from "4k data, 128 byte oob" to "2k >>>> data, 64 >>>> byte oob, 2k data, 64 byte oob" the marker is no longer in the >>>> oob. On >>>> first use we need to migrate the markers so that they are still in >>>> the oob. >>> Ah, I see, thanks. Are you planning to implement in-kernel migration or >>> use a user-space tool? >> That's the kind of answer I was hoping to get from Shuo. :-) > OK, I try to do this. Wait for a couple of days. > > -LiuShuo I found it's too complex to do the migration in Linux driver. Maybe we can add a uboot command (e.g. nand bbmigrate) to do it, once is enough. And let user ensure it been completed before linux use the Nand flash chip. Even if we don't do the migration, the bad block also can be marked as bad by wearing. So, do we really need to take much time to implement it ? (code looks too complex.) -LiuShuo >> Most likely is a firmware-based tool, but I'd like there to be some way >> for the tool to mark that this has happened, so that the Linux driver >> can refuse to do non-raw accesses to a chip that isn't marked as having >> been migrated (or at least yell loudly in the log). >> >> Speaking of raw accesses, these are currently broken in the eLBC >> driver... we need some way for the generic layer to tell us what kind of >> access it is before the transaction starts, not once it wants to read >> out the buffer (unless we add more hacks to delay the start of a read >> transaction until first buffer access...). We'd be better off with a >> high-level "read page/write page" function that does the whole thing >> (not just buffer access, but command issuance as well). >> >> -Scott >
On 12/14/2011 02:41 AM, LiuShuo wrote: > 于 2011年12月13日 10:46, LiuShuo 写道: >> 于 2011年12月13日 05:30, Scott Wood 写道: >>> On 12/12/2011 03:19 PM, Artem Bityutskiy wrote: >>>> On Mon, 2011-12-12 at 15:15 -0600, Scott Wood wrote: >>>>> NAND chips come from the factory with bad blocks marked at a certain >>>>> offset into each page. This offset is normally in the OOB area, but >>>>> since we change the layout from "4k data, 128 byte oob" to "2k >>>>> data, 64 >>>>> byte oob, 2k data, 64 byte oob" the marker is no longer in the >>>>> oob. On >>>>> first use we need to migrate the markers so that they are still in >>>>> the oob. >>>> Ah, I see, thanks. Are you planning to implement in-kernel migration or >>>> use a user-space tool? >>> That's the kind of answer I was hoping to get from Shuo. :-) >> OK, I try to do this. Wait for a couple of days. >> >> -LiuShuo > I found it's too complex to do the migration in Linux driver. > > Maybe we can add a uboot command (e.g. nand bbmigrate) to do it, once is > enough. Any reason not to do it automatically on the first U-Boot bad block scan, if the flash isn't marked as already migrated? Further discussion on the details of how to do it in U-Boot should move to the U-Boot list. > And let user ensure it been completed before linux use the Nand flash chip. I don't want to trust the user here. It's too easy to skip it, and things will appear to work, but have subtle problems. > Even if we don't do the migration, the bad block also can be marked as bad > by wearing. So, do we really need to take much time to implement it ? > (code looks too complex.) It is not acceptable to ignore factory bad block markers just because some methods of using the flash may eventually detect an error (possibly after data is lost -- no guarantee that the badness is ECC-correctable) and mark the block bad again. If you don't feel up to the task, I can look at it, but won't have time until January. -Scott
On 12/13/2011 09:41 PM, LiuShuo wrote: > 于 2011年12月13日 05:09, Artem Bityutskiy 写道: >> On Tue, 2011-12-06 at 18:09 -0600, Scott Wood wrote: >>> On 12/03/2011 10:31 PM, shuo.liu@freescale.com wrote: >>>> From: Liu Shuo<shuo.liu@freescale.com> >>>> >>>> Freescale FCM controller has a 2K size limitation of buffer RAM. In >>>> order >>>> to support the Nand flash chip whose page size is larger than 2K bytes, >>>> we read/write 2k data repeatedly by issuing FIR_OP_RB/FIR_OP_WB and >>>> save >>>> them to a large buffer. >>>> >>>> Signed-off-by: Liu Shuo<shuo.liu@freescale.com> >>>> --- >>>> v3: >>>> -remove page_size of struct fsl_elbc_mtd. >>>> -do a oob write by NAND_CMD_RNDIN. >>>> >>>> drivers/mtd/nand/fsl_elbc_nand.c | 243 >>>> ++++++++++++++++++++++++++++++++++---- >>>> 1 files changed, 218 insertions(+), 25 deletions(-) >>> What is the plan for bad block marker migration. > I think we can use a special bbt pattern to indicate whether migration > has been done. > (we needn't to define another marker) > > Do the migration our chip->scan_bbt as follow : > > /* > * this pattern indicate that the bad block information has been migrated, > * if this isn't found, we do the migration. > */ > static u8 migrated_bbt_pattern[] = {'M', 'b', 'b', 't', '0' }; > > static int fsl_elbc_bbt(struct mtd_info *mtd) > { > if (!check_migrated_bbt_pattern()) > bad_block_info_migtrate(); > > nand_default_bbt(mtd); /* default function in nand_bbt.c */ > } Hmm. This is OK as long as the bad block table never gets erased (which could happen if a user wants it reconstructed, such as if buggy software makes a mess of it on a developer's board). If it gets erased, we'll end up migrating again -- and the place that factory bad block markers would have been in is now data, so all blocks that have been written to will show up as bad unless they happen to have 0xff at the right place. How about a marker that is compatible with the bbt, so the same block can be used in production (where scrubbing the bbt should never happen), but that does not have to imply that the block is a bbt (so a developer that might want to erase the bbt can set the mark elsewhere, preferably just before the bbt)? Or have two versions of the marker, one that is also a bbt marker and one that is not. When scanning the bbt, the driver would look for one of these markers from the end of the chip backward. If not found, it concludes the chip is unmigrated. In U-Boot, this would trigger a migration (or a message to run a migration command). In Linux (and U-Boot if migration is a separate command that has not been run) an unmigrated flash could be read-only, with the possible exception of raw accesses if needed to support an external migration tool. -Scott
On Thu, Dec 15, 2011 at 4:15 AM, Scott Wood <scottwood@freescale.com> wrote: > On 12/14/2011 02:41 AM, LiuShuo wrote: >> 于 2011年12月13日 10:46, LiuShuo 写道: >>> 于 2011年12月13日 05:30, Scott Wood 写道: >>>> On 12/12/2011 03:19 PM, Artem Bityutskiy wrote: >>>>> On Mon, 2011-12-12 at 15:15 -0600, Scott Wood wrote: >>>>>> NAND chips come from the factory with bad blocks marked at a certain >>>>>> offset into each page. This offset is normally in the OOB area, but >>>>>> since we change the layout from "4k data, 128 byte oob" to "2k >>>>>> data, 64 >>>>>> byte oob, 2k data, 64 byte oob" the marker is no longer in the >>>>>> oob. On >>>>>> first use we need to migrate the markers so that they are still in >>>>>> the oob. >>>>> Ah, I see, thanks. Are you planning to implement in-kernel migration or >>>>> use a user-space tool? >>>> That's the kind of answer I was hoping to get from Shuo. :-) >>> OK, I try to do this. Wait for a couple of days. >>> >>> -LiuShuo >> I found it's too complex to do the migration in Linux driver. >> >> Maybe we can add a uboot command (e.g. nand bbmigrate) to do it, once is >> enough. > > Any reason not to do it automatically on the first U-Boot bad block > scan, if the flash isn't marked as already migrated? > > Further discussion on the details of how to do it in U-Boot should move > to the U-Boot list. The limitation of the proposed bad block marker migration is that you need to make sure the migration is done and only done once. If it is done more than once, the factory bad block marker is totally messed up. It requires a complex mechanism to automatically guarantee the migration is only done once, and it still won't be 100% safe. I would suggest we use a much easier compromise that we form the BBT base on the factory bad block marker on first use of the flash, and after that the factory bad block marker is dropped. We just relies on the BBT for information about bad blocks. Although by doing so we can't regenerate the BBT again, as there is mirror for the BBT I don't think we have too much risk. - Leo
On 12/14/2011 10:59 PM, Li Yang wrote: > The limitation of the proposed bad block marker migration is that you > need to make sure the migration is done and only done once. If it is > done more than once, the factory bad block marker is totally messed > up. It requires a complex mechanism to automatically guarantee the > migration is only done once, and it still won't be 100% safe. > > I would suggest we use a much easier compromise that we form the BBT > base on the factory bad block marker on first use of the flash, and > after that the factory bad block marker is dropped. We just relies on > the BBT for information about bad blocks. Although by doing so we > can't regenerate the BBT again, as there is mirror for the BBT I > don't think we have too much risk. I have corrupted the BBT too often during development (e.g. a bug makes all accesses fail, so the upper layers decide to mark everything bad) to be comfortable with this. Elsewhere in the thread I suggested a way to let the marker be in either the bbt or in a dedicated block, depending on whether it's a development situation where the BBT needs to be erasable. -Scott
于 2011年12月15日 04:15, Scott Wood 写道: > On 12/14/2011 02:41 AM, LiuShuo wrote: >> 于 2011年12月13日 10:46, LiuShuo 写道: >>> 于 2011年12月13日 05:30, Scott Wood 写道: >>>> On 12/12/2011 03:19 PM, Artem Bityutskiy wrote: >>>>> On Mon, 2011-12-12 at 15:15 -0600, Scott Wood wrote: >>>>>> NAND chips come from the factory with bad blocks marked at a certain >>>>>> offset into each page. This offset is normally in the OOB area, but >>>>>> since we change the layout from "4k data, 128 byte oob" to "2k >>>>>> data, 64 >>>>>> byte oob, 2k data, 64 byte oob" the marker is no longer in the >>>>>> oob. On >>>>>> first use we need to migrate the markers so that they are still in >>>>>> the oob. >>>>> Ah, I see, thanks. Are you planning to implement in-kernel migration or >>>>> use a user-space tool? >>>> That's the kind of answer I was hoping to get from Shuo. :-) >>> OK, I try to do this. Wait for a couple of days. >>> >>> -LiuShuo >> I found it's too complex to do the migration in Linux driver. >> >> Maybe we can add a uboot command (e.g. nand bbmigrate) to do it, once is >> enough. > Any reason not to do it automatically on the first U-Boot bad block > scan, if the flash isn't marked as already migrated? > > Further discussion on the details of how to do it in U-Boot should move > to the U-Boot list. > >> And let user ensure it been completed before linux use the Nand flash chip. > I don't want to trust the user here. It's too easy to skip it, and > things will appear to work, but have subtle problems. > >> Even if we don't do the migration, the bad block also can be marked as bad >> by wearing. So, do we really need to take much time to implement it ? >> (code looks too complex.) > It is not acceptable to ignore factory bad block markers just because > some methods of using the flash may eventually detect an error (possibly > after data is lost -- no guarantee that the badness is ECC-correctable) > and mark the block bad again. > > If you don't feel up to the task, I can look at it, but won't have time > until January. hi Scott, It's really hard to me and I have much other works to do now. Thanks for your help. hi Artem, Could this patch be applied now and we make a independent patch for bad block information migration later? -LiuShuo > -Scott
On 12/15/2011 08:44 PM, LiuShuo wrote: > hi Artem, > Could this patch be applied now and we make a independent patch for bad > block information > migration later? This patch is not safe to use without migration. -Scott
On Mon, 2011-12-12 at 15:30 -0600, Scott Wood wrote: > On 12/12/2011 03:19 PM, Artem Bityutskiy wrote: > > On Mon, 2011-12-12 at 15:15 -0600, Scott Wood wrote: > >> NAND chips come from the factory with bad blocks marked at a certain > >> offset into each page. This offset is normally in the OOB area, but > >> since we change the layout from "4k data, 128 byte oob" to "2k data, 64 > >> byte oob, 2k data, 64 byte oob" the marker is no longer in the oob. On > >> first use we need to migrate the markers so that they are still in the oob. > > > > Ah, I see, thanks. Are you planning to implement in-kernel migration or > > use a user-space tool? > > That's the kind of answer I was hoping to get from Shuo. :-) > > Most likely is a firmware-based tool, but I'd like there to be some way > for the tool to mark that this has happened, so that the Linux driver > can refuse to do non-raw accesses to a chip that isn't marked as having > been migrated (or at least yell loudly in the log). > > Speaking of raw accesses, these are currently broken in the eLBC > driver... we need some way for the generic layer to tell us what kind of > access it is before the transaction starts, not once it wants to read > out the buffer (unless we add more hacks to delay the start of a read > transaction until first buffer access...). We'd be better off with a > high-level "read page/write page" function that does the whole thing > (not just buffer access, but command issuance as well). It looks like currently you can re-define chip->read_page, so I guess you should rework MTD and make chip->write_page re-definable?
On Sat, Dec 17, 2011 at 1:59 AM, Scott Wood <scottwood@freescale.com> wrote: > On 12/15/2011 08:44 PM, LiuShuo wrote: >> hi Artem, >> Could this patch be applied now and we make a independent patch for bad >> block information >> migration later? > > This patch is not safe to use without migration. Hi Scott, We agree it's not entirely safe without migrating the bad block flag. But let's consider two sides of the situation. Firstly, it's only unsafe when there is a need to re-built the Bad Block Table from scratch(old BBT broken). But currently there is no easy way to do that(re-build BBT on demand), which means it's not a common problem that we can easily address now. Secondly, even if the previous said problem happens(BBT broken). We can still recover all the data if we overrule the bad block flag. Only the card is not so good to be used again, however, it can be used if we take the risk of losing data from errors that ECC can't notice(low possibility too). Finally, I don't think this is a blocker issue but a better to have enhancement. - Leo
On 12/19/2011 05:05 AM, Li Yang wrote: > On Sat, Dec 17, 2011 at 1:59 AM, Scott Wood <scottwood@freescale.com> wrote: >> On 12/15/2011 08:44 PM, LiuShuo wrote: >>> hi Artem, >>> Could this patch be applied now and we make a independent patch for bad >>> block information >>> migration later? >> >> This patch is not safe to use without migration. > > Hi Scott, > > We agree it's not entirely safe without migrating the bad block flag. > But let's consider two sides of the situation. > > Firstly, it's only unsafe when there is a need to re-built the Bad > Block Table from scratch(old BBT broken). No, it's unsafe in the presence of bad blocks. The BBT erasure issue relates to how me mark the flash as migrated, not whether we migrate in the first place. > But currently there is no > easy way to do that(re-build BBT on demand), You scrub the blocks with U-Boot. It's not supposed to be *easy*, it's a developer recovery mechanism. > Secondly, even if the previous said problem happens(BBT broken). We > can still recover all the data if we overrule the bad block flag. How so? The bad block markers -- including ones legitimately written to the BBT after the fact -- are used for block skipping with certain types of writes. Without the knowledge of which blocks were marked bad, how do we know which blocks were skipped? > Only the card is not so good to be used again, That's a pretty crappy thing to happen every time you hit a bug during development. But again, that's irrelevant to whether this patch should be applied as-is, because we currently don't have any bad block migration at all. > however, it can be used > if we take the risk of losing data from errors that ECC can't > notice(low possibility too). Can you quantify "low possibility" here? Note that any block that *was* marked bad will have a multi-bit error from the marker itself, since it will be embedded in the main data area. > Finally, I don't think this is a blocker issue but a better to have enhancement. No, it is not an enhancement. Processing bad block markers correctly is a fundamental requirement. And if anyone *does* start using it right away, then we'll have to deal with their complaints if we start checking for a migration marker later. Why is it so critical that it be merged now, and not in a few weeks (or next merge window) when I have a chance to do the migration code (assuming nobody else does it first) and add a suitable check for the migration marker in the Linux driver? -Scott
On 12/17/2011 08:35 AM, Artem Bityutskiy wrote: > On Mon, 2011-12-12 at 15:30 -0600, Scott Wood wrote: >> On 12/12/2011 03:19 PM, Artem Bityutskiy wrote: >>> On Mon, 2011-12-12 at 15:15 -0600, Scott Wood wrote: >>>> NAND chips come from the factory with bad blocks marked at a certain >>>> offset into each page. This offset is normally in the OOB area, but >>>> since we change the layout from "4k data, 128 byte oob" to "2k data, 64 >>>> byte oob, 2k data, 64 byte oob" the marker is no longer in the oob. On >>>> first use we need to migrate the markers so that they are still in the oob. >>> >>> Ah, I see, thanks. Are you planning to implement in-kernel migration or >>> use a user-space tool? >> >> That's the kind of answer I was hoping to get from Shuo. :-) >> >> Most likely is a firmware-based tool, but I'd like there to be some way >> for the tool to mark that this has happened, so that the Linux driver >> can refuse to do non-raw accesses to a chip that isn't marked as having >> been migrated (or at least yell loudly in the log). >> >> Speaking of raw accesses, these are currently broken in the eLBC >> driver... we need some way for the generic layer to tell us what kind of >> access it is before the transaction starts, not once it wants to read >> out the buffer (unless we add more hacks to delay the start of a read >> transaction until first buffer access...). We'd be better off with a >> high-level "read page/write page" function that does the whole thing >> (not just buffer access, but command issuance as well). > > It looks like currently you can re-define chip->read_page, so I guess > you should rework MTD and make chip->write_page re-definable? Unless something has changed very recently, there is no chip->read_page or chip->write_page. There is chip->ecc.read_page and chip->ecc.write_page, but they are too low-level. What we'd need to replace is a portion of nand_do_read_ops()/nand_do_write_ops(). -Scott
On 12/19/2011 12:38 PM, Scott Wood wrote: > On 12/17/2011 08:35 AM, Artem Bityutskiy wrote: >> It looks like currently you can re-define chip->read_page, so I guess >> you should rework MTD and make chip->write_page re-definable? > > Unless something has changed very recently, there is no chip->read_page > or chip->write_page. There is chip->ecc.read_page and > chip->ecc.write_page, but they are too low-level. What we'd need to > replace is a portion of nand_do_read_ops()/nand_do_write_ops(). Sorry, chip->write_page does exist -- it's chip->read_page that would need to be made similarly redefinable. -Scott
On Tue, Dec 20, 2011 at 12:47 AM, Scott Wood <scottwood@freescale.com> wrote: > On 12/19/2011 05:05 AM, Li Yang wrote: >> On Sat, Dec 17, 2011 at 1:59 AM, Scott Wood <scottwood@freescale.com> wrote: >>> On 12/15/2011 08:44 PM, LiuShuo wrote: >>>> hi Artem, >>>> Could this patch be applied now and we make a independent patch for bad >>>> block information >>>> migration later? >>> >>> This patch is not safe to use without migration. >> >> Hi Scott, >> >> We agree it's not entirely safe without migrating the bad block flag. >> But let's consider two sides of the situation. >> >> Firstly, it's only unsafe when there is a need to re-built the Bad >> Block Table from scratch(old BBT broken). > > No, it's unsafe in the presence of bad blocks. > Instead of migrating the factory bad block markers I proposed to modify the code of building BBT to make it different for 4K page, so that the default BBT can correctly covers the factory bad blocks. It is the easiest way with nearly no harm to the functionality. If you look at nand_default_block_markbad() in current implementation of Linux MTD. If we have set NAND_BBT_USE_FLASH option, which we did, the bad block information in only updated in BBT not the oob area of the first two pages of the bad block. That means we are currently only relies on the BBT for bad blocks. If the BBT is created, the factory bad block markers can be ignored, IMO. > The BBT erasure issue relates to how me mark the flash as migrated, not > whether we migrate in the first place. It is connected to whether we do the migration at all. I mentioned in earlier mail that if we are doing the migration, we need to make sure the migration only happens once. And it need to be done before the flash is used for the first time and before BBT is created. If we can't guarantee these condition, we are marking good blocks as bad by doing the migration. Even worse than doing nothing. > >> But currently there is no >> easy way to do that(re-build BBT on demand), > > You scrub the blocks with U-Boot. It's not supposed to be *easy*, it's > a developer recovery mechanism. Scrub clears the factory bad block markers also. It is the same result after scrub whether or not we migrated the factory bad block markers. > >> Secondly, even if the previous said problem happens(BBT broken). We >> can still recover all the data if we overrule the bad block flag. > > How so? The bad block markers -- including ones legitimately written to > the BBT after the fact -- are used for block skipping with certain types > of writes. Without the knowledge of which blocks were marked bad, how > do we know which blocks were skipped? This is not supposed to be *easy*. We might get more information in the file system level. Or we check the content of the blocks. > >> Only the card is not so good to be used again, > > That's a pretty crappy thing to happen every time you hit a bug during > development. > > But again, that's irrelevant to whether this patch should be applied > as-is, because we currently don't have any bad block migration at all. > >> however, it can be used >> if we take the risk of losing data from errors that ECC can't >> notice(low possibility too). > > Can you quantify "low possibility" here? > > Note that any block that *was* marked bad will have a multi-bit error > from the marker itself, since it will be embedded in the main data area. I found the definition of bad block from one NAND chip manual: Bad Blocks are blocks that contain one or more invalid bits whose reliability is not guaranteed. There is no mentioning that the bad block has to have multi-bit error. Although the factory bad blocks might have worse error than wear-off bad blocks, it's not what I can tell. > >> Finally, I don't think this is a blocker issue but a better to have enhancement. > > No, it is not an enhancement. Processing bad block markers correctly is > a fundamental requirement. And if anyone *does* start using it right > away, then we'll have to deal with their complaints if we start checking > for a migration marker later. I agree in some extend. I suggested to have the code of creating correct BBT for 4k page on first use, but not doing the migration. Given the code we have right now. We don't take more risk than before, and take no functionality lose. > > Why is it so critical that it be merged now, and not in a few weeks (or > next merge window) when I have a chance to do the migration code > (assuming nobody else does it first) and add a suitable check for the > migration marker in the Linux driver? A few weeks might be ok. But I feared that the merge can be further delayed and might finally goes no where. And as I argued above, I'm not sure if migrating is necessary in the first place. In general. We are not trying to get unqualified code merged. But I also don't agree we need to perfect all things before any of the code can be merged. My understanding is that even if certain code is not complete in feature or have certain drawbacks, if the current chunk provided some useful features and the drawbacks are acceptable, we should merge them and add more enhancements incrementally in the future. Some people don't have the luck to work on one thing for a long time, and can't possibly finish all the enhancements in one go. It's beneficial to merge part of the whole picture if it is acceptable rather than wait for an uncertain time for all to be finished. - Leo
On 12/20/2011 03:08 AM, Li Yang wrote: > On Tue, Dec 20, 2011 at 12:47 AM, Scott Wood <scottwood@freescale.com> wrote: >> On 12/19/2011 05:05 AM, Li Yang wrote: >>> On Sat, Dec 17, 2011 at 1:59 AM, Scott Wood <scottwood@freescale.com> wrote: >>>> On 12/15/2011 08:44 PM, LiuShuo wrote: >>>>> hi Artem, >>>>> Could this patch be applied now and we make a independent patch for bad >>>>> block information >>>>> migration later? >>>> >>>> This patch is not safe to use without migration. >>> >>> Hi Scott, >>> >>> We agree it's not entirely safe without migrating the bad block flag. >>> But let's consider two sides of the situation. >>> >>> Firstly, it's only unsafe when there is a need to re-built the Bad >>> Block Table from scratch(old BBT broken). >> >> No, it's unsafe in the presence of bad blocks. >> > > Instead of migrating the factory bad block markers I proposed to > modify the code of building BBT to make it different for 4K page, so > that the default BBT can correctly covers the factory bad blocks. It > is the easiest way with nearly no harm to the functionality. Even if we were to agree to that (I disagree with "nearly no harm"), this patch doesn't implement that. As is, this patch simply ignores the issue. Note that besides possibly tossing away bad block information during development, the BBT-only approach will not work for booting from NAND, as we don't use the BBT in that case (need to keep the code minimal to fit the 4k boot block). Yes, this ignores blocks that were marked bad by software, but that's usually OK since that part of the chip isn't managed by a software layer such as jffs2 that will mark blocks as bad. >> The BBT erasure issue relates to how me mark the flash as migrated, not >> whether we migrate in the first place. > > It is connected to whether we do the migration at all. I mentioned in > earlier mail that if we are doing the migration, we need to make sure > the migration only happens once. And it need to be done before the > flash is used for the first time and before BBT is created. If we > can't guarantee these condition, we are marking good blocks as bad by > doing the migration. Even worse than doing nothing. You also can't do the special BBT scan once the flash has been written to with normal data. This patch does not implement the special BBT scan. >>> But currently there is no >>> easy way to do that(re-build BBT on demand), >> >> You scrub the blocks with U-Boot. It's not supposed to be *easy*, it's >> a developer recovery mechanism. > > Scrub clears the factory bad block markers also. Only if you scrub bad blocks. I was talking about scrubbing the BBT specifically, not the entire chip. >>> Secondly, even if the previous said problem happens(BBT broken). We >>> can still recover all the data if we overrule the bad block flag. >> >> How so? The bad block markers -- including ones legitimately written to >> the BBT after the fact -- are used for block skipping with certain types >> of writes. Without the knowledge of which blocks were marked bad, how >> do we know which blocks were skipped? > > This is not supposed to be *easy*. We might get more information in > the file system level. Or we check the content of the blocks. If you need to take special, non-automatic steps to recover the data, that counts as data loss. >>> however, it can be used >>> if we take the risk of losing data from errors that ECC can't >>> notice(low possibility too). >> >> Can you quantify "low possibility" here? >> >> Note that any block that *was* marked bad will have a multi-bit error >> from the marker itself, since it will be embedded in the main data area. > > I found the definition of bad block from one NAND chip manual: Bad > Blocks are blocks that contain one or more invalid bits whose > reliability is not guaranteed. > > There is no mentioning that the bad block has to have multi-bit error. "It's not guaranteed to fail" is rather different from "low possibility of failure". > Although the factory bad blocks might have worse error than wear-off > bad blocks, it's not what I can tell. Why would they have such a mechanism to mark blocks bad, if it's not needed? >> Why is it so critical that it be merged now, and not in a few weeks (or >> next merge window) when I have a chance to do the migration code >> (assuming nobody else does it first) and add a suitable check for the >> migration marker in the Linux driver? > > A few weeks might be ok. But I feared that the merge can be further > delayed and might finally goes no where. And I fear that the bad block handling will be forgotten about if enough gets merged for this to sort-of work. > And as I argued above, I'm not sure if migrating is necessary in the first place. > > In general. We are not trying to get unqualified code merged. But I > also don't agree we need to perfect all things before any of the code > can be merged. I'm not asking for it to be perfect -- this just seems like a difficult thing to fix once people start using the feature (similar to getting a userspace API merged, we want to get it right first), and I'm not comfortable with the risks of people using it without bad block handling. > My understanding is that even if certain code is not > complete in feature or have certain drawbacks, if the current chunk > provided some useful features and the drawbacks are acceptable, Whether "the drawbacks are acceptable" is the issue. -Scott
diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c index d634c5f..a92411a 100644 --- a/drivers/mtd/nand/fsl_elbc_nand.c +++ b/drivers/mtd/nand/fsl_elbc_nand.c @@ -55,7 +55,6 @@ struct fsl_elbc_mtd { struct device *dev; int bank; /* Chip select bank number */ u8 __iomem *vbase; /* Chip select base virtual address */ - int page_size; /* NAND page size (0=512, 1=2048) */ unsigned int fmr; /* FCM Flash Mode Register value */ }; @@ -75,6 +74,8 @@ struct fsl_elbc_fcm_ctrl { unsigned int use_mdr; /* Non zero if the MDR is to be set */ unsigned int oob; /* Non zero if operating on OOB data */ unsigned int counter; /* counter for the initializations */ + + char *buffer; /* just be used when pagesize > 2048 */ }; /* These map to the positions used by the FCM hardware ECC generator */ @@ -150,6 +151,42 @@ static struct nand_bbt_descr bbt_mirror_descr = { }; /*=================================*/ +static void io_to_buffer(struct mtd_info *mtd, int subpage, int oob) +{ + struct nand_chip *chip = mtd->priv; + struct fsl_elbc_mtd *priv = chip->priv; + struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = priv->ctrl->nand; + void *src, *dst; + int len = (oob ? 64 : 2048); + + if (oob) + dst = elbc_fcm_ctrl->buffer + mtd->writesize + subpage * 64; + else + dst = elbc_fcm_ctrl->buffer + subpage * 2048; + + src = elbc_fcm_ctrl->addr + (oob ? 2048 : 0); + memcpy_fromio(dst, src, len); +} + +static void buffer_to_io(struct mtd_info *mtd, int subpage, int oob) +{ + struct nand_chip *chip = mtd->priv; + struct fsl_elbc_mtd *priv = chip->priv; + struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = priv->ctrl->nand; + void *src, *dst; + int len = (oob ? 64 : 2048); + + if (oob) + src = elbc_fcm_ctrl->buffer + mtd->writesize + subpage * 64; + else + src = elbc_fcm_ctrl->buffer + subpage * 2048; + + dst = elbc_fcm_ctrl->addr + (oob ? 2048 : 0); + memcpy_toio(dst, src, len); + + /* See the in_8() in fsl_elbc_write_buf() */ + in_8(elbc_fcm_ctrl->addr + (oob ? 2111 : 2047)); +} /* * Set up the FCM hardware block and page address fields, and the fcm @@ -166,7 +203,7 @@ static void set_addr(struct mtd_info *mtd, int column, int page_addr, int oob) elbc_fcm_ctrl->page = page_addr; - if (priv->page_size) { + if (mtd->writesize >= 2048) { /* * large page size chip : FPAR[PI] save the lowest 6 bits, * FBAR[BLK] save the other bits. @@ -193,7 +230,7 @@ static void set_addr(struct mtd_info *mtd, int column, int page_addr, int oob) /* for OOB data point to the second half of the buffer */ if (oob) - elbc_fcm_ctrl->index += priv->page_size ? 2048 : 512; + elbc_fcm_ctrl->index += mtd->writesize; dev_vdbg(priv->dev, "set_addr: bank=%d, " "elbc_fcm_ctrl->addr=0x%p (0x%p), " @@ -272,13 +309,14 @@ static int fsl_elbc_run_command(struct mtd_info *mtd) return 0; } -static void fsl_elbc_do_read(struct nand_chip *chip, int oob) +static void fsl_elbc_do_read(struct mtd_info *mtd, int oob) { + struct nand_chip *chip = mtd->priv; struct fsl_elbc_mtd *priv = chip->priv; struct fsl_lbc_ctrl *ctrl = priv->ctrl; struct fsl_lbc_regs __iomem *lbc = ctrl->regs; - if (priv->page_size) { + if (mtd->writesize >= 2048) { out_be32(&lbc->fir, (FIR_OP_CM0 << FIR_OP0_SHIFT) | (FIR_OP_CA << FIR_OP1_SHIFT) | @@ -311,6 +349,7 @@ static void fsl_elbc_cmdfunc(struct mtd_info *mtd, unsigned int command, struct fsl_lbc_ctrl *ctrl = priv->ctrl; struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = ctrl->nand; struct fsl_lbc_regs __iomem *lbc = ctrl->regs; + int i, n; elbc_fcm_ctrl->use_mdr = 0; @@ -337,8 +376,29 @@ static void fsl_elbc_cmdfunc(struct mtd_info *mtd, unsigned int command, elbc_fcm_ctrl->read_bytes = mtd->writesize + mtd->oobsize; elbc_fcm_ctrl->index += column; - fsl_elbc_do_read(chip, 0); + fsl_elbc_do_read(mtd, 0); fsl_elbc_run_command(mtd); + + if (mtd->writesize <= 2048) + return; + + /* Continue to read the rest bytes if writesize > 2048 */ + io_to_buffer(mtd, 0, 0); + io_to_buffer(mtd, 0, 1); + + out_be32(&lbc->fir, FIR_OP_RB << FIR_OP1_SHIFT); + + n = mtd->writesize / 2048; + for (i = 1; i < n; i++) { + /* + * Maybe there are some reasons of FCM hardware timing, + * we must insert a FIR_OP_NOP(0x00) before FIR_OP_RB. + */ + fsl_elbc_run_command(mtd); + io_to_buffer(mtd, i, 0); + io_to_buffer(mtd, i, 1); + } + return; /* READOOB reads only the OOB because no ECC is performed. */ @@ -347,13 +407,37 @@ static void fsl_elbc_cmdfunc(struct mtd_info *mtd, unsigned int command, "fsl_elbc_cmdfunc: NAND_CMD_READOOB, page_addr:" " 0x%x, column: 0x%x.\n", page_addr, column); - out_be32(&lbc->fbcr, mtd->oobsize - column); - set_addr(mtd, column, page_addr, 1); + if (mtd->writesize <= 2048) { + out_be32(&lbc->fbcr, mtd->oobsize - column); + set_addr(mtd, column, page_addr, 1); + } else { + out_be32(&lbc->fbcr, 64); + set_addr(mtd, 0, page_addr, 1); + elbc_fcm_ctrl->index += column; + } elbc_fcm_ctrl->read_bytes = mtd->writesize + mtd->oobsize; - fsl_elbc_do_read(chip, 1); + fsl_elbc_do_read(mtd, 1); fsl_elbc_run_command(mtd); + + if (mtd->writesize <= 2048) + return; + + if (column < 64) + io_to_buffer(mtd, 0, 1); + + out_be32(&lbc->fbcr, 2112); + out_be32(&lbc->fir, FIR_OP_RB << FIR_OP1_SHIFT); + out_be32(&lbc->fpar, in_be32(&lbc->fpar) & ~FPAR_LP_MS); + + n = mtd->writesize / 2048; + for (i = 1; i < n; i++) { + fsl_elbc_run_command(mtd); + if (column < (64 * (i + 1))) + io_to_buffer(mtd, i, 1); + } + return; /* READID must read all 5 possible bytes while CEB is active */ @@ -429,7 +513,17 @@ static void fsl_elbc_cmdfunc(struct mtd_info *mtd, unsigned int command, (NAND_CMD_SEQIN << FCR_CMD2_SHIFT) | (NAND_CMD_PAGEPROG << FCR_CMD3_SHIFT); - if (priv->page_size) { + if (mtd->writesize > 2048) { + /* writesize > 2048 */ + out_be32(&lbc->fir, + (FIR_OP_CM2 << FIR_OP0_SHIFT) | + (FIR_OP_CA << FIR_OP1_SHIFT) | + (FIR_OP_PA << FIR_OP2_SHIFT) | + (FIR_OP_WB << FIR_OP3_SHIFT)); + + if (elbc_fcm_ctrl->oob) + fcr |= NAND_CMD_RNDIN << FCR_CMD0_SHIFT; + } else if (mtd->writesize == 2048) { out_be32(&lbc->fir, (FIR_OP_CM2 << FIR_OP0_SHIFT) | (FIR_OP_CA << FIR_OP1_SHIFT) | @@ -464,6 +558,7 @@ static void fsl_elbc_cmdfunc(struct mtd_info *mtd, unsigned int command, /* PAGEPROG reuses all of the setup from SEQIN and adds the length */ case NAND_CMD_PAGEPROG: { + int pos; dev_vdbg(priv->dev, "fsl_elbc_cmdfunc: NAND_CMD_PAGEPROG " "writing %d bytes.\n", elbc_fcm_ctrl->index); @@ -473,13 +568,72 @@ static void fsl_elbc_cmdfunc(struct mtd_info *mtd, unsigned int command, * write so the HW generates the ECC. */ if (elbc_fcm_ctrl->oob || elbc_fcm_ctrl->column != 0 || - elbc_fcm_ctrl->index != mtd->writesize + mtd->oobsize) - out_be32(&lbc->fbcr, - elbc_fcm_ctrl->index - elbc_fcm_ctrl->column); - else + elbc_fcm_ctrl->index != mtd->writesize + mtd->oobsize) { + if (elbc_fcm_ctrl->oob && mtd->writesize > 2048) { + out_be32(&lbc->fbcr, 64); + } else { + out_be32(&lbc->fbcr, elbc_fcm_ctrl->index + - elbc_fcm_ctrl->column); + } + } else { out_be32(&lbc->fbcr, 0); + } + + if (mtd->writesize > 2048) { + if (!elbc_fcm_ctrl->oob) + buffer_to_io(mtd, 0, 0); + buffer_to_io(mtd, 0, 1); + } fsl_elbc_run_command(mtd); + + if (mtd->writesize <= 2048) + return; + + n = mtd->writesize / 2048; + + if (elbc_fcm_ctrl->oob) { + pos = 2048; + out_be32(&lbc->fir, + (FIR_OP_CM0 << FIR_OP0_SHIFT) | + (FIR_OP_UA << FIR_OP1_SHIFT) | + (FIR_OP_UA << FIR_OP2_SHIFT) | + (FIR_OP_WB << FIR_OP3_SHIFT)); + + for (i = 1; i < n; i++) { + pos += 2112; + elbc_fcm_ctrl->mdr = pos; + elbc_fcm_ctrl->use_mdr = 1; + if (i == n - 1) { + out_be32(&lbc->fir, + (FIR_OP_CM0 << FIR_OP1_SHIFT) | + (FIR_OP_UA << FIR_OP2_SHIFT) | + (FIR_OP_UA << FIR_OP3_SHIFT) | + (FIR_OP_WB << FIR_OP4_SHIFT) | + (FIR_OP_CM3 << FIR_OP5_SHIFT) | + (FIR_OP_CW1 << FIR_OP6_SHIFT) | + (FIR_OP_RS << FIR_OP7_SHIFT)); + } + buffer_to_io(mtd, i, 1); + fsl_elbc_run_command(mtd); + } + } else { + out_be32(&lbc->fir, FIR_OP_WB << FIR_OP1_SHIFT); + for (i = 1; i < n; i++) { + if (i == n - 1) { + elbc_fcm_ctrl->use_mdr = 1; + out_be32(&lbc->fir, + (FIR_OP_WB << FIR_OP1_SHIFT) | + (FIR_OP_CM3 << FIR_OP2_SHIFT) | + (FIR_OP_CW1 << FIR_OP3_SHIFT) | + (FIR_OP_RS << FIR_OP4_SHIFT)); + } + buffer_to_io(mtd, i, 0); + buffer_to_io(mtd, i, 1); + fsl_elbc_run_command(mtd); + } + } + return; } @@ -500,6 +654,7 @@ static void fsl_elbc_cmdfunc(struct mtd_info *mtd, unsigned int command, * write-protected, even when it is not. */ setbits8(elbc_fcm_ctrl->addr, NAND_STATUS_WP); + elbc_fcm_ctrl->buffer[0] = in_8(elbc_fcm_ctrl->addr); return; /* RESET without waiting for the ready line */ @@ -548,7 +703,14 @@ static void fsl_elbc_write_buf(struct mtd_info *mtd, const u8 *buf, int len) len = bufsize - elbc_fcm_ctrl->index; } - memcpy_toio(&elbc_fcm_ctrl->addr[elbc_fcm_ctrl->index], buf, len); + if (mtd->writesize > 2048) { + memcpy(&elbc_fcm_ctrl->buffer[elbc_fcm_ctrl->index], + buf, len); + } else { + memcpy_toio(&elbc_fcm_ctrl->addr[elbc_fcm_ctrl->index], + buf, len); + } + /* * This is workaround for the weird elbc hangs during nand write, * Scott Wood says: "...perhaps difference in how long it takes a @@ -572,8 +734,13 @@ static u8 fsl_elbc_read_byte(struct mtd_info *mtd) struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = priv->ctrl->nand; /* If there are still bytes in the FCM, then use the next byte. */ - if (elbc_fcm_ctrl->index < elbc_fcm_ctrl->read_bytes) - return in_8(&elbc_fcm_ctrl->addr[elbc_fcm_ctrl->index++]); + if (elbc_fcm_ctrl->index < elbc_fcm_ctrl->read_bytes) { + int index = elbc_fcm_ctrl->index++; + if (mtd->writesize > 2048) + return elbc_fcm_ctrl->buffer[index]; + else + return in_8(&elbc_fcm_ctrl->addr[index]); + } dev_err(priv->dev, "read_byte beyond end of buffer\n"); return ERR_BYTE; @@ -594,7 +761,13 @@ static void fsl_elbc_read_buf(struct mtd_info *mtd, u8 *buf, int len) avail = min((unsigned int)len, elbc_fcm_ctrl->read_bytes - elbc_fcm_ctrl->index); - memcpy_fromio(buf, &elbc_fcm_ctrl->addr[elbc_fcm_ctrl->index], avail); + if (mtd->writesize > 2048) { + memcpy(buf, &elbc_fcm_ctrl->buffer[elbc_fcm_ctrl->index], + avail); + } else { + memcpy_fromio(buf, &elbc_fcm_ctrl->addr[elbc_fcm_ctrl->index], + avail); + } elbc_fcm_ctrl->index += avail; if (len > avail) @@ -630,10 +803,17 @@ static int fsl_elbc_verify_buf(struct mtd_info *mtd, const u_char *buf, int len) return -EINVAL; } - for (i = 0; i < len; i++) - if (in_8(&elbc_fcm_ctrl->addr[elbc_fcm_ctrl->index + i]) - != buf[i]) - break; + if (mtd->writesize > 2048) { + for (i = 0; i < len; i++) + if (elbc_fcm_ctrl->buffer[elbc_fcm_ctrl->index + i] + != buf[i]) + break; + } else { + for (i = 0; i < len; i++) + if (in_8(&elbc_fcm_ctrl->addr[elbc_fcm_ctrl->index + i]) + != buf[i]) + break; + } elbc_fcm_ctrl->index += len; return i == len && elbc_fcm_ctrl->status == LTESR_CC ? 0 : -EIO; @@ -714,10 +894,9 @@ static int fsl_elbc_chip_init_tail(struct mtd_info *mtd) /* adjust Option Register and ECC to match Flash page size */ if (mtd->writesize == 512) { - priv->page_size = 0; clrbits32(&lbc->bank[priv->bank].or, OR_FCM_PGS); - } else if (mtd->writesize == 2048) { - priv->page_size = 1; + } else if (mtd->writesize >= 2048 && mtd->writesize <= 16 * 1024) { + setbits32(&lbc->bank[priv->bank].or, OR_FCM_PGS); /* adjust ecc setup if needed */ if ((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) == @@ -891,6 +1070,19 @@ static int __devinit fsl_elbc_nand_probe(struct platform_device *pdev) goto err; } elbc_fcm_ctrl->counter++; + /* + * Freescale FCM controller has a 2K size limitation of buffer + * RAM, so elbc_fcm_ctrl->buffer have to be used if writesize + * of chip is greater than 2048. + * We malloc a large enough buffer (maximum page size is 16K). + */ + elbc_fcm_ctrl->buffer = kmalloc(1024 * 16 + 1024, GFP_KERNEL); + if (!elbc_fcm_ctrl->buffer) { + dev_err(dev, "failed to allocate memory\n"); + mutex_unlock(&fsl_elbc_nand_mutex); + ret = -ENOMEM; + goto err; + } spin_lock_init(&elbc_fcm_ctrl->controller.lock); init_waitqueue_head(&elbc_fcm_ctrl->controller.wq); @@ -960,6 +1152,7 @@ static int fsl_elbc_nand_remove(struct platform_device *pdev) elbc_fcm_ctrl->counter--; if (!elbc_fcm_ctrl->counter) { fsl_lbc_ctrl_dev->nand = NULL; + kfree(elbc_fcm_ctrl->buffer); kfree(elbc_fcm_ctrl); } mutex_unlock(&fsl_elbc_nand_mutex);