[PATCH v2 1/2] mtd: rawnand: fsl_ifc: fix FSL NAND driver to read
Wan, Jane (Nokia - US/Sunnyvale)
jane.wan at nokia.com
Thu May 3 18:35:37 PDT 2018
Hi Miquel and Boris,
Thank you for your response. The following is the reposting the v2 version of the patch (also in the attachment).
Subject: [PATCH v2 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 at 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);
--
1.7.9.5
Best regards,
Jane
> -----Original Message-----
> From: Miquel Raynal [mailto:miquel.raynal at bootlin.com]
> Sent: Wednesday, May 02, 2018 1:10 AM
> To: Wan, Jane (Nokia - US/Sunnyvale) <jane.wan at nokia.com>
> Cc: Boris Brezillon <Boris.Brezillon at bootlin.com>; dwmw2 at infradead.org;
> computersforpeace at gmail.com; Bos, Ties (Nokia - US/Sunnyvale)
> <ties.bos at nokia.com>; linux-mtd at lists.infradead.org; linux-
> kernel at vger.kernel.org; richard at nod.at; marek.vasut at gmail.com;
> yamada.masahiro at socionext.com; prabhakar.kushwaha at nxp.com;
> shawnguo at kernel.org; jagdish.gediya at nxp.com;
> shreeya.patel23498 at gmail.com
> Subject: Re: [PATCH 1/2] Fix FSL NAND driver to read all ONFI parameter pages
>
> Hi Jane,
>
> On Tue, 1 May 2018 05:01:23 +0000, "Wan, Jane (Nokia - US/Sunnyvale)"
> <jane.wan at 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.
[Jane] Thank you for the info. I'm reposting the v2 version of the patch.
>
> >
> > 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.
[Jane] I understand. I can try to investigate it. The change is nontrivial. Our current
Kernel is based on v4.1.8 coming from vendors BSP. The testing of massive changes
in the latest kernel version may also be a challenge. I wish the current patch still could
help someone who may encounter the same problem we had recently with the bad
NAND chips.
>
>
> Thanks,
> Miquèl
>
> >
> > Best regards,
> > Jane
> >
> > Updated patch:
> > From 701de4146aa6355c951e97a77476e12d2da56d42 Mon Sep 17 00:00:00
> 2001
> > From: Jane Wan <Jane.Wan at 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 at 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);
>
>
>
> --
> Miquel Raynal, Bootlin (formerly Free Electrons) Embedded Linux and Kernel
> engineering https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-mtd-rawnand-fsl_ifc-fix-FSL-NAND-driver-to-read-all-.patch
Type: application/octet-stream
Size: 1926 bytes
Desc: 0001-mtd-rawnand-fsl_ifc-fix-FSL-NAND-driver-to-read-all-.patch
URL: <http://lists.infradead.org/pipermail/linux-mtd/attachments/20180504/6766f94c/attachment.obj>
More information about the linux-mtd
mailing list