Messages in this thread | | | Date | Wed, 23 Nov 2011 09:56:50 +0800 | From | LiuShuo <> | Subject | Re: [PATCH v3 3/3] mtd/nand : workaround for Freescale FCM to support large-page Nand chip |
| |
于 2011年11月23日 07:55, Scott Wood 写道: > On 11/15/2011 03:29 AM, b35362@freescale.com wrote: >> From: Liu Shuo<b35362@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> >> Signed-off-by: Shengzhou Liu<Shengzhou.Liu@freescale.com> >> Signed-off-by: Li Yang<leoli@freescale.com> >> --- >> drivers/mtd/nand/fsl_elbc_nand.c | 216 +++++++++++++++++++++++++++++++++++--- >> 1 files changed, 199 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c >> index c2c231b..415f87e 100644 >> --- a/drivers/mtd/nand/fsl_elbc_nand.c >> +++ b/drivers/mtd/nand/fsl_elbc_nand.c >> @@ -55,7 +55,9 @@ 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) */ >> + int page_size; /* NAND page size (0=512, 1=2048, 2=4096...), >> + * the mutiple of 2048. >> + */ > That "..." isn't very descriptive. What happens with 8192-byte pages? > Is it 3 or 4? > > Please just get rid of this and use mtd->writesize. > >> + for (i = 1; i< priv->page_size; i++) { >> + /* >> + * Maybe there are some reasons of FCM hardware timming, >> + * we must insert a FIR_OP_NOP(0x00) before FIR_OP_RB. >> + */ > s/timming/timing/ > >> /* PAGEPROG reuses all of the setup from SEQIN and adds the length */ >> case NAND_CMD_PAGEPROG: { >> + int len; >> dev_vdbg(priv->dev, >> "fsl_elbc_cmdfunc: NAND_CMD_PAGEPROG " >> "writing %d bytes.\n", elbc_fcm_ctrl->index); >> - >> /* if the write did not start at 0 or is not a full page >> * then set the exact length, otherwise use a full page >> * write so the HW generates the ECC. >> */ >> - if (elbc_fcm_ctrl->oob || elbc_fcm_ctrl->column != 0 || >> + if (elbc_fcm_ctrl->column>= mtd->writesize) { >> + /* write oob */ >> + if (priv->page_size> 1) { >> + /* when pagesize of chip is greater than 2048, >> + * we have to write full page to write spare >> + * region, so we fill '0xff' to main region >> + * and some bytes of spare region which we >> + * don't want to rewrite. >> + * (write '1' won't change the original value) >> + */ >> + memset(elbc_fcm_ctrl->buffer, 0xff, >> + elbc_fcm_ctrl->column); > I don't like relying on this -- can we use RNDIN instead to do a > discontiguous write? > >> + len = 2112; > len = min(elbc_fcm_ctrl->index, 2112); > >> + } else >> + len = mtd->writesize + mtd->oobsize - >> + elbc_fcm_ctrl->column; >> + out_be32(&lbc->fbcr, len); > len = elbc_fcm_ctrl->index - elbc_fcm_ctrl->column; > > Use braces on both sides of the if/else if it's needed on one side. > >> + } else if (elbc_fcm_ctrl->column != 0 || >> elbc_fcm_ctrl->index != mtd->writesize + mtd->oobsize) >> out_be32(&lbc->fbcr, elbc_fcm_ctrl->index); > This should have set fbcr to index - column as well (after adjusting -- > though really it's not a supported use case. We should only be seeing > column != 0 for oob. > >> @@ -625,10 +776,16 @@ 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; > Please use braces around multiline if/for bodies, even if they're > technically a single statement -- especially when you've got a dangling > else. > >> elbc_fcm_ctrl->index += len; >> return i == len&& elbc_fcm_ctrl->status == LTESR_CC ? 0 : -EIO; >> @@ -657,6 +814,7 @@ static int fsl_elbc_chip_init_tail(struct mtd_info *mtd) >> struct fsl_elbc_mtd *priv = chip->priv; >> struct fsl_lbc_ctrl *ctrl = priv->ctrl; >> struct fsl_lbc_regs __iomem *lbc = ctrl->regs; >> + struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = ctrl->nand; >> unsigned int al; >> >> /* calculate FMR Address Length field */ >> @@ -707,12 +865,17 @@ static int fsl_elbc_chip_init_tail(struct mtd_info *mtd) >> dev_dbg(priv->dev, "fsl_elbc_init: mtd->oobsize = %d\n", >> mtd->oobsize); >> >> + kfree(elbc_fcm_ctrl->buffer); >> + elbc_fcm_ctrl->buffer = NULL; >> + >> /* 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) { >> + /* page_size = writesize / 2048 */ >> + priv->page_size = mtd->writesize>> 11; > Why not just write priv->page_size = mtd->writesize / 2048 in the code > rather than the comment (it compiles to the same code)? Other than > because you were requested to remove priv->page_size, that is. :-) > >> setbits32(&lbc->bank[priv->bank].or, OR_FCM_PGS); >> /* adjust ecc setup if needed */ >> if ((in_be32(&lbc->bank[priv->bank].br)& BR_DECC) == >> @@ -723,6 +886,14 @@ static int fsl_elbc_chip_init_tail(struct mtd_info *mtd) >> &fsl_elbc_oob_lp_eccm0; >> chip->badblock_pattern =&largepage_memorybased; >> } >> + >> + /* re-malloc if pagesize> 2048*/ >> + if (mtd->writesize> 2048) { >> + elbc_fcm_ctrl->buffer = kmalloc(mtd->writesize + >> + mtd->oobsize, GFP_KERNEL); >> + if (!elbc_fcm_ctrl->buffer) >> + return -ENOMEM; >> + } >> } else { >> dev_err(priv->dev, >> "fsl_elbc_init: page size %d is not supported\n", > buffer is a controller-wide resource, but you're setting it based on the > most-recently-probed NAND chip. It should be large enough to > accommodate the largest page size in use on any connected chip. Even if > all chips in the system have the same page size, you're introducing a > race if a previously-probed chip is already in use (the controller lock > is not held here). > > Just allocate a buffer large enough for a 16K page size in the main init > function, and print an error in the tail if you encounter a larger page > size. > >> @@ -886,6 +1057,17 @@ 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 at this point, because we >> + * don't know writesize before calling nand_scan(). We will >> + * re-malloc later if needed. >> + */ >> + elbc_fcm_ctrl->buffer = kmalloc(4096 * 6, GFP_KERNEL); >> + if (!elbc_fcm_ctrl->buffer) >> + return -ENOMEM; > Clean up properly if you fail to allocate the buffer. This includes > freeing elbc_fcm_ctrl, setting fsl_lbc_ctrl_dev->nand to NULL, and > releasing fsl_elbc_nand_mutex. > > -Scott Thanks for your suggestions. I will make it better.
-LiuShuo
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |