7 messages, 2 authors, 2012-01-04 · open the first message on its own page
From: Prabhakar Kushwaha <hidden>
Date: 2011-12-29 04:59:57
This Patch series takes care issues with Freescale IFC driver for supporting 2K page size NAND with ECC enabled. [PATCH 1/2] mtd/nand:Fix wrong address read in is_blank() Fix driver issue when ECC enabled. [PATCH 2/2] mtd/nand: Fix IFC driver to support 2K NAND page Fix driver during OOB updation
From: Prabhakar Kushwaha <hidden>
Date: 2011-12-29 04:59:59
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 <redacted> Signed-off-by: Prabhakar Kushwaha <redacted> --- 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;
@@ -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;
--
1.7.5.4
From: Prabhakar Kushwaha <hidden>
Date: 2011-12-29 05:00:04
1) OOB area should be updated irrespective of NAND page size. Earlier it was updated only for 512byte NAND page. 2) During OOB update fbcr should be equal to OOB size. Signed-off-by: Poonam Aggrwal <redacted> Signed-off-by: Prabhakar Kushwaha <redacted> --- git://git.kernel.org/pub/scm/linux/kernel/git/galak/powerpc.git (branch next) Tested on P1010RDB drivers/mtd/nand/fsl_ifc_nand.c | 20 ++++++++------------ 1 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
index 2df7206..2c02168 100644
--- a/drivers/mtd/nand/fsl_ifc_nand.c
+++ b/drivers/mtd/nand/fsl_ifc_nand.c@@ -439,20 +439,16 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command, out_be32(&ifc->ifc_nand.nand_fir1, (IFC_FIR_OP_CW1 << IFC_NAND_FIR1_OP5_SHIFT)); - if (column >= mtd->writesize) { - /* OOB area --> READOOB */ - column -= mtd->writesize; - nand_fcr0 |= NAND_CMD_READOOB << - IFC_NAND_FCR0_CMD0_SHIFT; - ifc_nand_ctrl->oob = 1; - } else if (column < 256) + if (column < 256) /* First 256 bytes --> READ0 */ nand_fcr0 |= NAND_CMD_READ0 << IFC_NAND_FCR0_CMD0_SHIFT; - else - /* Second 256 bytes --> READ1 */ - nand_fcr0 |= - NAND_CMD_READ1 << IFC_NAND_FCR0_CMD0_SHIFT; + } + + if (column >= mtd->writesize) { + /* OOB area --> READOOB */ + column -= mtd->writesize; + ifc_nand_ctrl->oob = 1; } out_be32(&ifc->ifc_nand.nand_fcr0, nand_fcr0);
@@ -465,7 +461,7 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command, int full_page; if (ifc_nand_ctrl->oob) { out_be32(&ifc->ifc_nand.nand_fbcr, - ifc_nand_ctrl->index); + ifc_nand_ctrl->index - ifc_nand_ctrl->column); full_page = 0; } else { out_be32(&ifc->ifc_nand.nand_fbcr, 0);
--
1.7.5.4
From: Scott Wood <hidden>
Date: 2012-01-03 19:50:02
On 12/28/2011 10:59 PM, Prabhakar Kushwaha wrote:
1) OOB area should be updated irrespective of NAND page size. Earlier it was updated only for 512byte NAND page. 2) During OOB update fbcr should be equal to OOB size. Signed-off-by: Poonam Aggrwal <redacted> Signed-off-by: Prabhakar Kushwaha <redacted> --- git://git.kernel.org/pub/scm/linux/kernel/git/galak/powerpc.git (branch next)
The IFC driver hasn't been merged into that tree that I can see.
Tested on P1010RDB drivers/mtd/nand/fsl_ifc_nand.c | 20 ++++++++------------ 1 files changed, 8 insertions(+), 12 deletions(-)diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c index 2df7206..2c02168 100644 --- a/drivers/mtd/nand/fsl_ifc_nand.c +++ b/drivers/mtd/nand/fsl_ifc_nand.c@@ -439,20 +439,16 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command, out_be32(&ifc->ifc_nand.nand_fir1, (IFC_FIR_OP_CW1 << IFC_NAND_FIR1_OP5_SHIFT)); - if (column >= mtd->writesize) { - /* OOB area --> READOOB */ - column -= mtd->writesize; - nand_fcr0 |= NAND_CMD_READOOB << - IFC_NAND_FCR0_CMD0_SHIFT; - ifc_nand_ctrl->oob = 1; - } else if (column < 256) + if (column < 256) /* First 256 bytes --> READ0 */ nand_fcr0 |= NAND_CMD_READ0 << IFC_NAND_FCR0_CMD0_SHIFT; - else - /* Second 256 bytes --> READ1 */ - nand_fcr0 |= - NAND_CMD_READ1 << IFC_NAND_FCR0_CMD0_SHIFT; + } + + if (column >= mtd->writesize) { + /* OOB area --> READOOB */ + column -= mtd->writesize; + ifc_nand_ctrl->oob = 1; }
Where is NAND_CMD_READOOB going to be set in the small-page case?
The small-page code should read something like:
if (column >= mtd->writesize) {
nand_fcr0 |=
NAND_CMD_READOOB << IFC_NAND_FCR0_CMD0_SHIFT;
} else {
nand_fcr0 |=
NAND_CMD_READ0 << IFC_NAND_FCR0_CMD0_SHIFT;
}
It looks like we can get rid of ctrl->column, BTW.
-Scott
From: Scott Wood <hidden>
Date: 2012-01-03 20:24:13
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 <redacted> Signed-off-by: Prabhakar Kushwaha <redacted> --- 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
From: Prabhakar <hidden>
Date: 2012-01-04 04:36:25
On Wednesday 04 January 2012 01:54 AM, Scott Wood wrote:
On 12/28/2011 10:59 PM, Prabhakar Kushwaha wrote:quoted
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<redacted> Signed-off-by: Prabhakar Kushwaha<redacted> --- 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.
quoted
@@ -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
From: Prabhakar <hidden>
Date: 2012-01-04 04:59:49
On Wednesday 04 January 2012 01:19 AM, Scott Wood wrote:
On 12/28/2011 10:59 PM, Prabhakar Kushwaha wrote:quoted
1) OOB area should be updated irrespective of NAND page size. Earlier it was updated only for 512byte NAND page. 2) During OOB update fbcr should be equal to OOB size. Signed-off-by: Poonam Aggrwal<redacted> Signed-off-by: Prabhakar Kushwaha<redacted> --- git://git.kernel.org/pub/scm/linux/kernel/git/galak/powerpc.git (branch next)The IFC driver hasn't been merged into that tree that I can see.
This patch is created on top of IFC driver patch (already floated in mailing list). Please find there link: http://patchwork.ozlabs.org/patch/133315/ http://patchwork.ozlabs.org/patch/133316/
quoted
Tested on P1010RDB drivers/mtd/nand/fsl_ifc_nand.c | 20 ++++++++------------ 1 files changed, 8 insertions(+), 12 deletions(-)diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c index 2df7206..2c02168 100644 --- a/drivers/mtd/nand/fsl_ifc_nand.c +++ b/drivers/mtd/nand/fsl_ifc_nand.c@@ -439,20 +439,16 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command, out_be32(&ifc->ifc_nand.nand_fir1, (IFC_FIR_OP_CW1<< IFC_NAND_FIR1_OP5_SHIFT)); - if (column>= mtd->writesize) { - /* OOB area --> READOOB */ - column -= mtd->writesize; - nand_fcr0 |= NAND_CMD_READOOB<< - IFC_NAND_FCR0_CMD0_SHIFT; - ifc_nand_ctrl->oob = 1; - } else if (column< 256) + if (column< 256) /* First 256 bytes --> READ0 */ nand_fcr0 |= NAND_CMD_READ0<< IFC_NAND_FCR0_CMD0_SHIFT; - else - /* Second 256 bytes --> READ1 */ - nand_fcr0 |= - NAND_CMD_READ1<< IFC_NAND_FCR0_CMD0_SHIFT; + } + + if (column>= mtd->writesize) { + /* OOB area --> READOOB */ + column -= mtd->writesize; + ifc_nand_ctrl->oob = 1; }Where is NAND_CMD_READOOB going to be set in the small-page case?
2K NAND flash does not require NAND_CMD_READOOB. So i thought same should be applied to 512byte NAND. but i am wrong. Thanks for pointing it out :)
The small-page code should read something like:
if (column>= mtd->writesize) {
nand_fcr0 |=
NAND_CMD_READOOB<< IFC_NAND_FCR0_CMD0_SHIFT;
} else {
nand_fcr0 |=
NAND_CMD_READ0<< IFC_NAND_FCR0_CMD0_SHIFT;
}
It looks like we can get rid of ctrl->column, BTW.I will take care this in next patch release --Prabhakar