| Message ID | 1325134779-3571-2-git-send-email-prabhakar@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 12/28/2011 10:59 PM, Prabhakar Kushwaha wrote: > IFC NAND Machine calculates ECC on 512byte sector. Same is taken care in > fsl_ifc_run_command() while ECC status verification. Here buffer number is > calculated assuming 512byte sector and same is passed to is_blank. > However in is_blank() buffer address is calculated using mdt->writesize which is > wrong. It should be calculated on basis of ecc sector size. > > Also, in fsl_ifc_run_command() bufferpage is calculated on the basis of ecc sector > size instead of hard coded value. > > Signed-off-by: Poonam Aggrwal <poonam.aggrwal@freescale.com> > Signed-off-by: Prabhakar Kushwaha <prabhakar@freescale.com> > --- > git://git.kernel.org/pub/scm/linux/kernel/git/galak/powerpc.git (branch next) > > Tested on P1010RDB > > drivers/mtd/nand/fsl_ifc_nand.c | 6 ++++-- > 1 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c > index 8475b88..2df7206 100644 > --- a/drivers/mtd/nand/fsl_ifc_nand.c > +++ b/drivers/mtd/nand/fsl_ifc_nand.c > @@ -191,7 +191,9 @@ static int is_blank(struct mtd_info *mtd, unsigned int bufnum) > { > struct nand_chip *chip = mtd->priv; > struct fsl_ifc_mtd *priv = chip->priv; > - u8 __iomem *addr = priv->vbase + bufnum * (mtd->writesize * 2); > + int bufperpage = mtd->writesize / chip->ecc.size; > + u8 __iomem *addr = priv->vbase + bufnum / bufperpage > + * (mtd->writesize * 2); > u32 __iomem *mainarea = (u32 *)addr; > u8 __iomem *oob = addr + mtd->writesize; > int i; This function should only be checking one ECC block, not the entire page. The caller is responsible for passing in the appropriate buffer numbers. I think what the current code needs is for (mtd->writesize * 2) to be replaced with chip->ecc.size, and for the calling code to multiply the starting bufnum by two. > @@ -273,7 +275,7 @@ static void fsl_ifc_run_command(struct mtd_info *mtd) > dev_err(priv->dev, "NAND Flash Write Protect Error\n"); > > if (nctrl->eccread) { > - int bufperpage = mtd->writesize / 512; > + int bufperpage = mtd->writesize / chip->ecc.size; > int bufnum = (nctrl->page & priv->bufnum_mask) * bufperpage; > int bufnum_end = bufnum + bufperpage - 1; > Currently this driver always sets chip->ecc.size to 512. If we want to support other ECC block sizes that future versions of IFC may have, can we calculate bufperpage during chip init (similar to bufnum_mask) to avoid the runtime division? It's probably not huge overhead compared to everything else we do per NAND page transfer, but still... -Scott
On Wednesday 04 January 2012 01:54 AM, Scott Wood wrote: > On 12/28/2011 10:59 PM, Prabhakar Kushwaha wrote: >> IFC NAND Machine calculates ECC on 512byte sector. Same is taken care in >> fsl_ifc_run_command() while ECC status verification. Here buffer number is >> calculated assuming 512byte sector and same is passed to is_blank. >> However in is_blank() buffer address is calculated using mdt->writesize which is >> wrong. It should be calculated on basis of ecc sector size. >> >> Also, in fsl_ifc_run_command() bufferpage is calculated on the basis of ecc sector >> size instead of hard coded value. >> >> Signed-off-by: Poonam Aggrwal<poonam.aggrwal@freescale.com> >> Signed-off-by: Prabhakar Kushwaha<prabhakar@freescale.com> >> --- >> git://git.kernel.org/pub/scm/linux/kernel/git/galak/powerpc.git (branch next) >> >> Tested on P1010RDB >> >> drivers/mtd/nand/fsl_ifc_nand.c | 6 ++++-- >> 1 files changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c >> index 8475b88..2df7206 100644 >> --- a/drivers/mtd/nand/fsl_ifc_nand.c >> +++ b/drivers/mtd/nand/fsl_ifc_nand.c >> @@ -191,7 +191,9 @@ static int is_blank(struct mtd_info *mtd, unsigned int bufnum) >> { >> struct nand_chip *chip = mtd->priv; >> struct fsl_ifc_mtd *priv = chip->priv; >> - u8 __iomem *addr = priv->vbase + bufnum * (mtd->writesize * 2); >> + int bufperpage = mtd->writesize / chip->ecc.size; >> + u8 __iomem *addr = priv->vbase + bufnum / bufperpage >> + * (mtd->writesize * 2); >> u32 __iomem *mainarea = (u32 *)addr; >> u8 __iomem *oob = addr + mtd->writesize; >> int i; > This function should only be checking one ECC block, not the entire > page. The caller is responsible for passing in the appropriate buffer > numbers. > > I think what the current code needs is for (mtd->writesize * 2) to be > replaced with chip->ecc.size, and for the calling code to multiply the > starting bufnum by two. Got your point :). I will take care in next patch version. >> @@ -273,7 +275,7 @@ static void fsl_ifc_run_command(struct mtd_info *mtd) >> dev_err(priv->dev, "NAND Flash Write Protect Error\n"); >> >> if (nctrl->eccread) { >> - int bufperpage = mtd->writesize / 512; >> + int bufperpage = mtd->writesize / chip->ecc.size; >> int bufnum = (nctrl->page& priv->bufnum_mask) * bufperpage; >> int bufnum_end = bufnum + bufperpage - 1; >> > Currently this driver always sets chip->ecc.size to 512. If we want to > support other ECC block sizes that future versions of IFC may have, can > we calculate bufperpage during chip init (similar to bufnum_mask) to > avoid the runtime division? It's probably not huge overhead compared to > everything else we do per NAND page transfer, but still... > Yes. I agree. We are working on this in order to support new controller version. --Prabhakar
diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c index 8475b88..2df7206 100644 --- a/drivers/mtd/nand/fsl_ifc_nand.c +++ b/drivers/mtd/nand/fsl_ifc_nand.c @@ -191,7 +191,9 @@ static int is_blank(struct mtd_info *mtd, unsigned int bufnum) { struct nand_chip *chip = mtd->priv; struct fsl_ifc_mtd *priv = chip->priv; - u8 __iomem *addr = priv->vbase + bufnum * (mtd->writesize * 2); + int bufperpage = mtd->writesize / chip->ecc.size; + u8 __iomem *addr = priv->vbase + bufnum / bufperpage + * (mtd->writesize * 2); u32 __iomem *mainarea = (u32 *)addr; u8 __iomem *oob = addr + mtd->writesize; int i; @@ -273,7 +275,7 @@ static void fsl_ifc_run_command(struct mtd_info *mtd) dev_err(priv->dev, "NAND Flash Write Protect Error\n"); if (nctrl->eccread) { - int bufperpage = mtd->writesize / 512; + int bufperpage = mtd->writesize / chip->ecc.size; int bufnum = (nctrl->page & priv->bufnum_mask) * bufperpage; int bufnum_end = bufnum + bufperpage - 1;