RE: [PATCH 1/2] Fix FSL NAND driver to read all ONFI parameter pages

[Wan, Jane (Nokia - US/Sunnyvale)]

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@xxxxxxxxx>
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@xxxxxxxxx>
---
 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@xxxxxxxxxxx] 
Sent: Saturday, April 28, 2018 4:42 AM
To: Wan, Jane (Nokia - US/Sunnyvale) <jane.wan@xxxxxxxxx>
Cc: dwmw2@xxxxxxxxxxxxx; computersforpeace@xxxxxxxxx; Bos, Ties (Nokia - US/Sunnyvale) <ties.bos@xxxxxxxxx>; linux-mtd@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Boris Brezillon <Boris.Brezillon@xxxxxxxxxxx>
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@xxxxxxxxx>

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: 0001-mtd-rawnand-fsl_ifc-fix-FSL-NAND-driver-to-read-all-.patch
Description: 0001-mtd-rawnand-fsl_ifc-fix-FSL-NAND-driver-to-read-all-.patch