Patchwork mtd: nand: cleanup ONFI printed errors, warnings

login
register
mail settings
Submitter Brian Norris
Date Sept. 11, 2013, 11:08 p.m.
Message ID <1378940894-27598-1-git-send-email-computersforpeace@gmail.com>
Download mbox | patch
Permalink /patch/274388/
State Accepted
Commit c7f23a70635895b5125aeb5593aaf8cb44d3a088
Headers show

Comments

Brian Norris - Sept. 11, 2013, 11:08 p.m.
The ONFI detection routine is too verbose in some cases and not verbose
enough in others. This patch refactors it to print only when there are
significant warnings/errors.

Probing in 16-bit mode:
  It is unnecessary to print until after the READID (address 20h)
  command. READID *has* to work properly in whatever bus width
  configuration we are in, or else no identification mode works. So we
  can silence some useless warnings on systems which come up in 16-bit
  mode and do not even respond with an O-N-F-I string.

Valid parameter page:
  Nobody needs to see this. Do we inform the user every time other
  hardware responds properly? Instead, add an error message if *no*
  uncorrupted parameter pages are found.

ONFI ECC:
  Most drivers don't yet use the reported minimum ECC values, so it
  shouldn't yet be a fatal condition if the extended parameter page is
  incorrect. But we should at least give a warning for the corner cases
  that we don't expect.

ONFI flash detected:
  Nobody needs to see this. This is the expected case, that we detect
  ONFI properly, or else it wasn't ONFI-compliant and is detected by
  some other routine.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Cc: Huang Shijie <b32955@freescale.com>
Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/nand_base.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)
Ezequiel Garcia - Sept. 12, 2013, 12:24 p.m.
On Wed, Sep 11, 2013 at 04:08:14PM -0700, Brian Norris wrote:
> The ONFI detection routine is too verbose in some cases and not verbose
> enough in others. This patch refactors it to print only when there are
> significant warnings/errors.
> 
> Probing in 16-bit mode:
>   It is unnecessary to print until after the READID (address 20h)
>   command. READID *has* to work properly in whatever bus width
>   configuration we are in, or else no identification mode works. So we
>   can silence some useless warnings on systems which come up in 16-bit
>   mode and do not even respond with an O-N-F-I string.
> 
> Valid parameter page:
>   Nobody needs to see this. Do we inform the user every time other
>   hardware responds properly? Instead, add an error message if *no*
>   uncorrupted parameter pages are found.
> 
> ONFI ECC:
>   Most drivers don't yet use the reported minimum ECC values, so it
>   shouldn't yet be a fatal condition if the extended parameter page is
>   incorrect. But we should at least give a warning for the corner cases
>   that we don't expect.
> 
> ONFI flash detected:
>   Nobody needs to see this. This is the expected case, that we detect
>   ONFI properly, or else it wasn't ONFI-compliant and is detected by
>   some other routine.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Cc: Huang Shijie <b32955@freescale.com>
> Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  drivers/mtd/nand/nand_base.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 7ed4841..d4578a1 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -2937,29 +2937,34 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
>  	int i;
>  	int val;
>  
> -	/* ONFI need to be probed in 8 bits mode, and 16 bits should be selected with NAND_BUSWIDTH_AUTO */
> -	if (chip->options & NAND_BUSWIDTH_16) {
> -		pr_err("Trying ONFI probe in 16 bits mode, aborting !\n");
> -		return 0;
> -	}
>  	/* Try ONFI for unknown chip or LP */
>  	chip->cmdfunc(mtd, NAND_CMD_READID, 0x20, -1);
>  	if (chip->read_byte(mtd) != 'O' || chip->read_byte(mtd) != 'N' ||
>  		chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I')
>  		return 0;
>  
> +	/*
> +	 * ONFI must be probed in 8-bit mode or with NAND_BUSWIDTH_AUTO, not
> +	 * with NAND_BUSWIDTH_16
> +	 */
> +	if (chip->options & NAND_BUSWIDTH_16) {
> +		pr_err("ONFI cannot be probed in 16-bit mode; aborting\n");
> +		return 0;
> +	}
> +
>  	chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
>  	for (i = 0; i < 3; i++) {
>  		chip->read_buf(mtd, (uint8_t *)p, sizeof(*p));
>  		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
>  				le16_to_cpu(p->crc)) {
> -			pr_info("ONFI param page %d valid\n", i);
>  			break;
>  		}
>  	}
>  
> -	if (i == 3)
> +	if (i == 3) {
> +		pr_err("Could not find valid ONFI parameter page; aborting\n");
>  		return 0;
> +	}
>  
>  	/* Check version */
>  	val = le16_to_cpu(p->revision);
> @@ -3011,10 +3016,11 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
>  
>  		/* The Extended Parameter Page is supported since ONFI 2.1. */
>  		if (nand_flash_detect_ext_param_page(mtd, chip, p))
> -			pr_info("Failed to detect the extended param page.\n");
> +			pr_warn("Failed to detect ONFI extended param page\n");
> +	} else {
> +		pr_warn("Could not retrieve ONFI ECC requirements\n");
>  	}
>  
> -	pr_info("ONFI flash detected\n");
>  	return 1;
>  }
>  

Looks good. I'd suggest to put:

  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

at the top of this file, to prefix all the messages with a nice "nand:"
string, but then you may want to refactor the "NAND device:" notification.
Brian Norris - Sept. 17, 2013, 1:03 a.m.
On Wed, Sep 11, 2013 at 04:08:14PM -0700, Brian Norris wrote:
> The ONFI detection routine is too verbose in some cases and not verbose
> enough in others. This patch refactors it to print only when there are
> significant warnings/errors.
> 
> Probing in 16-bit mode:
>   It is unnecessary to print until after the READID (address 20h)
>   command. READID *has* to work properly in whatever bus width
>   configuration we are in, or else no identification mode works. So we
>   can silence some useless warnings on systems which come up in 16-bit
>   mode and do not even respond with an O-N-F-I string.
> 
> Valid parameter page:
>   Nobody needs to see this. Do we inform the user every time other
>   hardware responds properly? Instead, add an error message if *no*
>   uncorrupted parameter pages are found.
> 
> ONFI ECC:
>   Most drivers don't yet use the reported minimum ECC values, so it
>   shouldn't yet be a fatal condition if the extended parameter page is
>   incorrect. But we should at least give a warning for the corner cases
>   that we don't expect.
> 
> ONFI flash detected:
>   Nobody needs to see this. This is the expected case, that we detect
>   ONFI properly, or else it wasn't ONFI-compliant and is detected by
>   some other routine.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Cc: Huang Shijie <b32955@freescale.com>
> Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

Applied to l2-mtd.git.

Brian

Patch

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 7ed4841..d4578a1 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2937,29 +2937,34 @@  static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
 	int i;
 	int val;
 
-	/* ONFI need to be probed in 8 bits mode, and 16 bits should be selected with NAND_BUSWIDTH_AUTO */
-	if (chip->options & NAND_BUSWIDTH_16) {
-		pr_err("Trying ONFI probe in 16 bits mode, aborting !\n");
-		return 0;
-	}
 	/* Try ONFI for unknown chip or LP */
 	chip->cmdfunc(mtd, NAND_CMD_READID, 0x20, -1);
 	if (chip->read_byte(mtd) != 'O' || chip->read_byte(mtd) != 'N' ||
 		chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I')
 		return 0;
 
+	/*
+	 * ONFI must be probed in 8-bit mode or with NAND_BUSWIDTH_AUTO, not
+	 * with NAND_BUSWIDTH_16
+	 */
+	if (chip->options & NAND_BUSWIDTH_16) {
+		pr_err("ONFI cannot be probed in 16-bit mode; aborting\n");
+		return 0;
+	}
+
 	chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
 	for (i = 0; i < 3; i++) {
 		chip->read_buf(mtd, (uint8_t *)p, sizeof(*p));
 		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
 				le16_to_cpu(p->crc)) {
-			pr_info("ONFI param page %d valid\n", i);
 			break;
 		}
 	}
 
-	if (i == 3)
+	if (i == 3) {
+		pr_err("Could not find valid ONFI parameter page; aborting\n");
 		return 0;
+	}
 
 	/* Check version */
 	val = le16_to_cpu(p->revision);
@@ -3011,10 +3016,11 @@  static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
 
 		/* The Extended Parameter Page is supported since ONFI 2.1. */
 		if (nand_flash_detect_ext_param_page(mtd, chip, p))
-			pr_info("Failed to detect the extended param page.\n");
+			pr_warn("Failed to detect ONFI extended param page\n");
+	} else {
+		pr_warn("Could not retrieve ONFI ECC requirements\n");
 	}
 
-	pr_info("ONFI flash detected\n");
 	return 1;
 }