| Message ID | 1524788396-32380-2-git-send-email-Jane.Wan@nokia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Fix fsl_ifc_nand reading ONFI parameters to meet ONFI spec | expand |
Hi Jane, You forgot to Cc the right maintainers, please use ./scripts/get_maintainer.pl for that. > Signed-off-by: Jane Wan <Jane.Wan@nokia.com> Please add a description of what your are doing in the commit message. The description in the cover letter is good, you can copy the relevant section here. > --- > drivers/mtd/nand/fsl_ifc_nand.c | 10 ++++++---- Also, just for you to know, files have moved in a raw/ subdirectory, so please rebase on top of 4.17-rc1 and prefix the commit title with "mtd: rawnand: fsl_ifc:". > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c > index ca36b35..a3cf6ca 100644 > --- a/drivers/mtd/nand/fsl_ifc_nand.c > +++ b/drivers/mtd/nand/fsl_ifc_nand.c > @@ -413,6 +413,7 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command, > struct fsl_ifc_mtd *priv = chip->priv; > struct fsl_ifc_ctrl *ctrl = priv->ctrl; > struct fsl_ifc_runtime __iomem *ifc = ctrl->rregs; > + int len; > > /* clear the read buffer */ > ifc_nand_ctrl->read_bytes = 0; > @@ -462,11 +463,12 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command, > ifc_out32(column, &ifc->ifc_nand.row3); > > /* > - * although currently it's 8 bytes for READID, we always read > - * the maximum 256 bytes(for PARAM) > + * For READID, read 8 bytes that are currently used. > + * For PARAM, read all 3 copies of 256-bytes pages. > */ > - ifc_out32(256, &ifc->ifc_nand.nand_fbcr); > - ifc_nand_ctrl->read_bytes = 256; > + len = (command == NAND_CMD_PARAM) ? (3 * 256) : 8; There is already a "command == NAND_CMD_PARAM" condition above, you might want to use it. > + ifc_out32(len, &ifc->ifc_nand.nand_fbcr); > + ifc_nand_ctrl->read_bytes = len; > > set_addr(mtd, 0, 0, 0); > fsl_ifc_run_command(mtd); The overall ->cmdfunc() approach of this driver is horrible. However this fixes its implementation to match the current state of the core, so I guess it is fine. Regards, Miquèl
Hi Jane, On Thu, 26 Apr 2018 17:19:55 -0700 Jane Wan <Jane.Wan@nokia.com> wrote: > Signed-off-by: Jane Wan <Jane.Wan@nokia.com> > --- > drivers/mtd/nand/fsl_ifc_nand.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c > index ca36b35..a3cf6ca 100644 > --- a/drivers/mtd/nand/fsl_ifc_nand.c > +++ b/drivers/mtd/nand/fsl_ifc_nand.c > @@ -413,6 +413,7 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command, > struct fsl_ifc_mtd *priv = chip->priv; > struct fsl_ifc_ctrl *ctrl = priv->ctrl; > struct fsl_ifc_runtime __iomem *ifc = ctrl->rregs; > + int len; > > /* clear the read buffer */ > ifc_nand_ctrl->read_bytes = 0; > @@ -462,11 +463,12 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command, > ifc_out32(column, &ifc->ifc_nand.row3); > > /* > - * although currently it's 8 bytes for READID, we always read > - * the maximum 256 bytes(for PARAM) > + * For READID, read 8 bytes that are currently used. > + * For PARAM, read all 3 copies of 256-bytes pages. > */ > - ifc_out32(256, &ifc->ifc_nand.nand_fbcr); > - ifc_nand_ctrl->read_bytes = 256; > + len = (command == NAND_CMD_PARAM) ? (3 * 256) : 8; > + ifc_out32(len, &ifc->ifc_nand.nand_fbcr); > + ifc_nand_ctrl->read_bytes = len; This driver really calls for a clean rework to transition to ->exec_op(). Guessing the amount of data to be read from ->cmdfunc() is broken and your patch series just shows how broken this is. What next? What if some NANDs have 4 or more copies of the param page? I'm still undecided whether I want to apply this patch. I guess having some guarantees that someone will actually work on implementing ->exec_op() in a near future would help me take this decision. Regards, Boris
Hi Miquèl and Boris, Thank you for your response and feedback. I've modified the fix based on your comments. Please see the updated patch file at the end of this message (also in attachment). My answers to your comments/questions are inline in the previous message. Here is the answer to Boris question in another email thread: > What if some NANDs have 4 or more copies of the param page? [Jane] The ONFI spec defines that the parameter page and its two redundant copies are mandatory. The additional redundant pages are optional. Currently, the FSL NAND driver only reads the first parameter page. This patch is to fix the driver to meet the mandatory requirement in the spec. We got a batch of particularly bad NAND chips recently and we needed these changes to make them work reliably over temperature. The patch was verified using these bad chips. Best regards, Jane Updated patch: From 701de4146aa6355c951e97a77476e12d2da56d42 Mon Sep 17 00:00:00 2001 From: Jane Wan <Jane.Wan@nokia.com> Date: Mon, 30 Apr 2018 13:30:46 -0700 Subject: [PATCH 1/2] mtd: rawnand: fsl_ifc: fix FSL NAND driver to read all ONFI parameter pages Per ONFI specification (Rev. 4.0), if the CRC of the first parameter page read is not valid, the host should read redundant parameter page copies. Fix FSL NAND driver to read the two redundant copies which are mandatory in the specification. Signed-off-by: Jane Wan <Jane.Wan@nokia.com> --- drivers/mtd/nand/raw/fsl_ifc_nand.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/drivers/mtd/nand/raw/fsl_ifc_nand.c b/drivers/mtd/nand/raw/fsl_ifc_nand.c index 61aae02..98aac1f 100644 --- a/drivers/mtd/nand/raw/fsl_ifc_nand.c +++ b/drivers/mtd/nand/raw/fsl_ifc_nand.c @@ -342,9 +342,16 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command, case NAND_CMD_READID: case NAND_CMD_PARAM: { + /* + * For READID, read 8 bytes that are currently used. + * For PARAM, read all 3 copies of 256-bytes pages. + */ + int len = 8; int timing = IFC_FIR_OP_RB; - if (command == NAND_CMD_PARAM) + if (command == NAND_CMD_PARAM) { timing = IFC_FIR_OP_RBCD; + len = 256 * 3; + } ifc_out32((IFC_FIR_OP_CW0 << IFC_NAND_FIR0_OP0_SHIFT) | (IFC_FIR_OP_UA << IFC_NAND_FIR0_OP1_SHIFT) | @@ -354,12 +361,8 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command, &ifc->ifc_nand.nand_fcr0); ifc_out32(column, &ifc->ifc_nand.row3); - /* - * although currently it's 8 bytes for READID, we always read - * the maximum 256 bytes(for PARAM) - */ - ifc_out32(256, &ifc->ifc_nand.nand_fbcr); - ifc_nand_ctrl->read_bytes = 256; + ifc_out32(len, &ifc->ifc_nand.nand_fbcr); + ifc_nand_ctrl->read_bytes = len; set_addr(mtd, 0, 0, 0); fsl_ifc_run_command(mtd);
Hi Jane, On Tue, 1 May 2018 05:01:23 +0000, "Wan, Jane (Nokia - US/Sunnyvale)" <jane.wan@nokia.com> wrote: > Hi Miquèl and Boris, > > Thank you for your response and feedback. I've modified the fix based on your comments. > Please see the updated patch file at the end of this message (also in attachment). > My answers to your comments/questions are inline in the previous message. Usually we answer inline in each e-mail, then we post a new version with a version counter incremented (use the '-v2- of 'git format-patch' to add the '[PATCH v2 x/y]' prefix automatically). Reposting is important as maintainers use 'patchwork' to follow the evolution of each patch. So in your case, nothing shows that you posted a v2. > > Here is the answer to Boris question in another email thread: > > > What if some NANDs have 4 or more copies of the param page? > [Jane] The ONFI spec defines that the parameter page and its two redundant copies are mandatory. > The additional redundant pages are optional. Currently, the FSL NAND driver only reads the first > parameter page. This patch is to fix the driver to meet the mandatory requirement in the spec. > We got a batch of particularly bad NAND chips recently and we needed these changes to make them > work reliably over temperature. The patch was verified using these bad chips. I think Boris' remark was much more general than just this use case. The whole logic of tailoring ->cmdfunc() in each driver by guessing the data length, while there is absolutely no indication of it in this hook is broken. The core is moving to a new interface called ->exec_op(), and we would welcome a migration of this driver to this hook, that would be much much more easy to maintain for everyone. Thanks, Miquèl > > Best regards, > Jane > > Updated patch: > From 701de4146aa6355c951e97a77476e12d2da56d42 Mon Sep 17 00:00:00 2001 > From: Jane Wan <Jane.Wan@nokia.com> > Date: Mon, 30 Apr 2018 13:30:46 -0700 > Subject: [PATCH 1/2] mtd: rawnand: fsl_ifc: fix FSL NAND driver to read all > ONFI parameter pages > > Per ONFI specification (Rev. 4.0), if the CRC of the first parameter page > read is not valid, the host should read redundant parameter page copies. > Fix FSL NAND driver to read the two redundant copies which are mandatory > in the specification. > > Signed-off-by: Jane Wan <Jane.Wan@nokia.com> > --- > drivers/mtd/nand/raw/fsl_ifc_nand.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/drivers/mtd/nand/raw/fsl_ifc_nand.c b/drivers/mtd/nand/raw/fsl_ifc_nand.c > index 61aae02..98aac1f 100644 > --- a/drivers/mtd/nand/raw/fsl_ifc_nand.c > +++ b/drivers/mtd/nand/raw/fsl_ifc_nand.c > @@ -342,9 +342,16 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command, > > case NAND_CMD_READID: > case NAND_CMD_PARAM: { > + /* > + * For READID, read 8 bytes that are currently used. > + * For PARAM, read all 3 copies of 256-bytes pages. > + */ > + int len = 8; > int timing = IFC_FIR_OP_RB; > - if (command == NAND_CMD_PARAM) > + if (command == NAND_CMD_PARAM) { > timing = IFC_FIR_OP_RBCD; > + len = 256 * 3; > + } > > ifc_out32((IFC_FIR_OP_CW0 << IFC_NAND_FIR0_OP0_SHIFT) | > (IFC_FIR_OP_UA << IFC_NAND_FIR0_OP1_SHIFT) | > @@ -354,12 +361,8 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command, > &ifc->ifc_nand.nand_fcr0); > ifc_out32(column, &ifc->ifc_nand.row3); > > - /* > - * although currently it's 8 bytes for READID, we always read > - * the maximum 256 bytes(for PARAM) > - */ > - ifc_out32(256, &ifc->ifc_nand.nand_fbcr); > - ifc_nand_ctrl->read_bytes = 256; > + ifc_out32(len, &ifc->ifc_nand.nand_fbcr); > + ifc_nand_ctrl->read_bytes = len; > > set_addr(mtd, 0, 0, 0); > fsl_ifc_run_command(mtd);
On Tue, 1 May 2018 05:01:23 +0000 "Wan, Jane (Nokia - US/Sunnyvale)" <jane.wan@nokia.com> wrote: > Hi Miquèl and Boris, > > Thank you for your response and feedback. I've modified the fix based on your comments. > Please see the updated patch file at the end of this message (also in attachment). > My answers to your comments/questions are inline in the previous message. > > Here is the answer to Boris question in another email thread: > > > What if some NANDs have 4 or more copies of the param page? > [Jane] The ONFI spec defines that the parameter page and its two redundant copies are mandatory. > The additional redundant pages are optional. Currently, the FSL NAND driver only reads the first > parameter page. This patch is to fix the driver to meet the mandatory requirement in the spec. > We got a batch of particularly bad NAND chips recently and we needed these changes to make them > work reliably over temperature. The patch was verified using these bad chips. And that proves my point. The core is reading 3 param pages [1], but since this driver was trying to guess how many bytes to read from ->cmdfunc() and did not guess correctly you ended up with a partially working implementation (works only if the first PARAM page is valid). Now, you fix it to read 3 PARAM pages, but what if we decide to read more to cope with MLC NANDs where even more copy are needed to have one valid version? You'll have to patch ->cmdfunc() again, just because you're trying to guess something that the core is supposed to tell you. [1]https://elixir.bootlin.com/linux/v4.17-rc3/source/drivers/mtd/nand/raw/nand_base.c#L5115
diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c index ca36b35..a3cf6ca 100644 --- a/drivers/mtd/nand/fsl_ifc_nand.c +++ b/drivers/mtd/nand/fsl_ifc_nand.c @@ -413,6 +413,7 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command, struct fsl_ifc_mtd *priv = chip->priv; struct fsl_ifc_ctrl *ctrl = priv->ctrl; struct fsl_ifc_runtime __iomem *ifc = ctrl->rregs; + int len; /* clear the read buffer */ ifc_nand_ctrl->read_bytes = 0; @@ -462,11 +463,12 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command, ifc_out32(column, &ifc->ifc_nand.row3); /* - * although currently it's 8 bytes for READID, we always read - * the maximum 256 bytes(for PARAM) + * For READID, read 8 bytes that are currently used. + * For PARAM, read all 3 copies of 256-bytes pages. */ - ifc_out32(256, &ifc->ifc_nand.nand_fbcr); - ifc_nand_ctrl->read_bytes = 256; + len = (command == NAND_CMD_PARAM) ? (3 * 256) : 8; + ifc_out32(len, &ifc->ifc_nand.nand_fbcr); + ifc_nand_ctrl->read_bytes = len; set_addr(mtd, 0, 0, 0); fsl_ifc_run_command(mtd);
Signed-off-by: Jane Wan <Jane.Wan@nokia.com> --- drivers/mtd/nand/fsl_ifc_nand.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)