LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Wan, Jane (Nokia - US/Sunnyvale)" <jane.wan@nokia.com>
To: Miquel Raynal <miquel.raynal@bootlin.com>,
	Boris Brezillon <Boris.Brezillon@bootlin.com>
Cc: "dwmw2@infradead.org" <dwmw2@infradead.org>,
	"computersforpeace@gmail.com" <computersforpeace@gmail.com>,
	"Bos, Ties (Nokia - US/Sunnyvale)" <ties.bos@nokia.com>,
	"linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"richard@nod.at" <richard@nod.at>,
	"marek.vasut@gmail.com" <marek.vasut@gmail.com>,
	"yamada.masahiro@socionext.com" <yamada.masahiro@socionext.com>,
	"prabhakar.kushwaha@nxp.com" <prabhakar.kushwaha@nxp.com>,
	"miquel.raynal@bootlin.com" <miquel.raynal@bootlin.com>,
	"shawnguo@kernel.org" <shawnguo@kernel.org>,
	"jagdish.gediya@nxp.com" <jagdish.gediya@nxp.com>,
	"shreeya.patel23498@gmail.com" <shreeya.patel23498@gmail.com>
Subject: RE: [PATCH 1/2] Fix FSL NAND driver to read all ONFI parameter pages
Date: Tue, 1 May 2018 05:01:23 +0000	[thread overview]
Message-ID: <VI1PR07MB161575ACAE380B18882E8EE981810@VI1PR07MB1615.eurprd07.prod.outlook.com> (raw)
In-Reply-To: <20180428134218.3ae857eb@xps13>

[-- Attachment #1: Type: text/plain, Size: 5887 bytes --]

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);
-- 
1.7.9.5


-----Original Message-----
From: Miquel Raynal [mailto:miquel.raynal@bootlin.com] 
Sent: Saturday, April 28, 2018 4:42 AM
To: Wan, Jane (Nokia - US/Sunnyvale) <jane.wan@nokia.com>
Cc: dwmw2@infradead.org; computersforpeace@gmail.com; Bos, Ties (Nokia - US/Sunnyvale) <ties.bos@nokia.com>; linux-mtd@lists.infradead.org; linux-kernel@vger.kernel.org; Boris Brezillon <Boris.Brezillon@bootlin.com>
Subject: Re: [PATCH 1/2] Fix FSL NAND driver to read all ONFI parameter pages

Hi Jane,

You forgot to Cc the right maintainers, please use ./scripts/get_maintainer.pl for that.
[Jane]  Added through 4.17-rc1 get_maintainer.pl.  I was running the script from an older kernel version we're using.

> 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.
[Jane]  Added.

> ---
>  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:".
[Jane] Thank you for the info.  I've rebased the change on top of 4.17-rc1 and modified the commit title as you suggested.

>  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.
[Jane] Done as suggested.

> +		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

--
Miquel Raynal, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com

[-- Attachment #2: 0001-mtd-rawnand-fsl_ifc-fix-FSL-NAND-driver-to-read-all-.patch --]
[-- Type: application/octet-stream, Size: 1923 bytes --]

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);
-- 
1.7.9.5


  reply	other threads:[~2018-05-01  5:01 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-27  0:19 [PATCH 0/2] Fix fsl_ifc_nand reading ONFI parameters to meet ONFI spec Jane Wan
2018-04-27  0:19 ` [PATCH 1/2] Fix FSL NAND driver to read all ONFI parameter pages Jane Wan
2018-04-28 11:42   ` Miquel Raynal
2018-05-01  5:01     ` Wan, Jane (Nokia - US/Sunnyvale) [this message]
2018-05-02  8:10       ` Miquel Raynal
2018-05-02 10:39       ` Boris Brezillon
2018-04-30 10:00   ` Boris Brezillon
2018-04-27  0:19 ` [PATCH 2/2] Use bit-wise majority to recover the contents of ONFI parameter Jane Wan
2018-04-28 12:06   ` Miquel Raynal
2018-05-01  5:33     ` Wan, Jane (Nokia - US/Sunnyvale)
2018-05-02 10:25   ` Boris Brezillon
2018-05-02 10:31     ` Boris Brezillon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=VI1PR07MB161575ACAE380B18882E8EE981810@VI1PR07MB1615.eurprd07.prod.outlook.com \
    --to=jane.wan@nokia.com \
    --cc=Boris.Brezillon@bootlin.com \
    --cc=computersforpeace@gmail.com \
    --cc=dwmw2@infradead.org \
    --cc=jagdish.gediya@nxp.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=marek.vasut@gmail.com \
    --cc=miquel.raynal@bootlin.com \
    --cc=prabhakar.kushwaha@nxp.com \
    --cc=richard@nod.at \
    --cc=shawnguo@kernel.org \
    --cc=shreeya.patel23498@gmail.com \
    --cc=ties.bos@nokia.com \
    --cc=yamada.masahiro@socionext.com \
    --subject='RE: [PATCH 1/2] Fix FSL NAND driver to read all ONFI parameter pages' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).