* [PATCH] mtd: nand: cleanup ONFI printed errors, warnings
@ 2013-09-11 23:08 Brian Norris
2013-09-12 12:24 ` Ezequiel Garcia
2013-09-17 1:03 ` Brian Norris
0 siblings, 2 replies; 5+ messages in thread
From: Brian Norris @ 2013-09-11 23:08 UTC (permalink / raw)
To: linux-mtd; +Cc: Huang Shijie, Brian Norris, Ezequiel Garcia
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;
}
--
1.8.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] mtd: nand: cleanup ONFI printed errors, warnings
2013-09-11 23:08 [PATCH] mtd: nand: cleanup ONFI printed errors, warnings Brian Norris
@ 2013-09-12 12:24 ` Ezequiel Garcia
2013-09-12 14:49 ` Ezequiel Garcia
2013-09-17 1:03 ` Brian Norris
1 sibling, 1 reply; 5+ messages in thread
From: Ezequiel Garcia @ 2013-09-12 12:24 UTC (permalink / raw)
To: Brian Norris; +Cc: Huang Shijie, linux-mtd
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.
--
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mtd: nand: cleanup ONFI printed errors, warnings
2013-09-12 12:24 ` Ezequiel Garcia
@ 2013-09-12 14:49 ` Ezequiel Garcia
2013-09-30 23:20 ` Brian Norris
0 siblings, 1 reply; 5+ messages in thread
From: Ezequiel Garcia @ 2013-09-12 14:49 UTC (permalink / raw)
To: Brian Norris; +Cc: Huang Shijie, linux-mtd, David Woodhouse, Artem Bityutskiy
On Thu, Sep 12, 2013 at 09:24:14AM -0300, Ezequiel Garcia wrote:
> 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.
FWIW, here's my proposal (which applies on top of this patch):
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index d4578a1..ff5bb5a 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -29,6 +29,8 @@
*
*/
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
#include <linux/module.h>
#include <linux/delay.h>
#include <linux/errno.h>
@@ -3449,10 +3451,11 @@ ident_done:
if (mtd->writesize > 512 && chip->cmdfunc == nand_command)
chip->cmdfunc = nand_command_lp;
- pr_info("NAND device: Manufacturer ID: 0x%02x, Chip ID: 0x%02x (%s %s),"
- " %dMiB, page size: %d, OOB size: %d\n",
- *maf_id, *dev_id, nand_manuf_ids[maf_idx].name,
- chip->onfi_version ? chip->onfi_params.model : type->name,
+ pr_info("device found, Manufacturer ID: 0x%02x, Chip ID: 0x%02x\n",
+ *maf_id, *dev_id);
+ pr_info("%s %s\n", nand_manuf_ids[maf_idx].name,
+ chip->onfi_version ? chip->onfi_params.model : type->name);
+ pr_info("%dMiB, page size: %d, OOB size: %d\n",
(int)(chip->chipsize >> 20), mtd->writesize, mtd->oobsize);
return type;
--
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] mtd: nand: cleanup ONFI printed errors, warnings
2013-09-11 23:08 [PATCH] mtd: nand: cleanup ONFI printed errors, warnings Brian Norris
2013-09-12 12:24 ` Ezequiel Garcia
@ 2013-09-17 1:03 ` Brian Norris
1 sibling, 0 replies; 5+ messages in thread
From: Brian Norris @ 2013-09-17 1:03 UTC (permalink / raw)
To: linux-mtd; +Cc: Huang Shijie, Ezequiel Garcia
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mtd: nand: cleanup ONFI printed errors, warnings
2013-09-12 14:49 ` Ezequiel Garcia
@ 2013-09-30 23:20 ` Brian Norris
0 siblings, 0 replies; 5+ messages in thread
From: Brian Norris @ 2013-09-30 23:20 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: Huang Shijie, linux-mtd@lists.infradead.org, David Woodhouse,
Artem Bityutskiy
On Thu, Sep 12, 2013 at 7:49 AM, Ezequiel Garcia
<ezequiel.garcia@free-electrons.com> wrote:
> On Thu, Sep 12, 2013 at 09:24:14AM -0300, Ezequiel Garcia wrote:
>> 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.
>
> FWIW, here's my proposal (which applies on top of this patch):
>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index d4578a1..ff5bb5a 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -29,6 +29,8 @@
> *
> */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> #include <linux/module.h>
> #include <linux/delay.h>
> #include <linux/errno.h>
> @@ -3449,10 +3451,11 @@ ident_done:
> if (mtd->writesize > 512 && chip->cmdfunc == nand_command)
> chip->cmdfunc = nand_command_lp;
>
> - pr_info("NAND device: Manufacturer ID: 0x%02x, Chip ID: 0x%02x (%s %s),"
> - " %dMiB, page size: %d, OOB size: %d\n",
> - *maf_id, *dev_id, nand_manuf_ids[maf_idx].name,
> - chip->onfi_version ? chip->onfi_params.model : type->name,
> + pr_info("device found, Manufacturer ID: 0x%02x, Chip ID: 0x%02x\n",
> + *maf_id, *dev_id);
> + pr_info("%s %s\n", nand_manuf_ids[maf_idx].name,
> + chip->onfi_version ? chip->onfi_params.model : type->name);
> + pr_info("%dMiB, page size: %d, OOB size: %d\n",
> (int)(chip->chipsize >> 20), mtd->writesize, mtd->oobsize);
>
> return type;
>
I just pushed Huang's changes, which conflict with this. If you still
want this, go ahead and send a proper, updated patch with description
and sign-off.
Thanks,
Brian
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-09-30 23:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-11 23:08 [PATCH] mtd: nand: cleanup ONFI printed errors, warnings Brian Norris
2013-09-12 12:24 ` Ezequiel Garcia
2013-09-12 14:49 ` Ezequiel Garcia
2013-09-30 23:20 ` Brian Norris
2013-09-17 1:03 ` Brian Norris
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox