Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1408057 > unrolled thread

[PATCH 00/15] mtd: nand: allow vendor specific detection/initialization

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/15] mtd: nand: allow vendor specific detection/initialization Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-05-27 15:00 +0200
    [PATCH 05/15] mtd: nand: add vendor specific initialization step Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-05-27 15:00 +0200
    [PATCH 13/15] mtd: nand: samsung: retrieve ECC requirements from extended ID Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-05-27 15:00 +0200
      Re: [PATCH 13/15] mtd: nand: samsung: retrieve ECC requirements from extended ID Valdis.Kletnieks@vt.edu - 2016-05-30 02:30 +0200
        Re: [PATCH 13/15] mtd: nand: samsung: retrieve ECC requirements  from extended ID Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-05-30 09:50 +0200
          Re: [PATCH 13/15] mtd: nand: samsung: retrieve ECC requirements from extended ID Valdis.Kletnieks@vt.edu - 2016-05-30 23:00 +0200
            Re: [PATCH 13/15] mtd: nand: samsung: retrieve ECC requirements  from extended ID Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-05-31 00:30 +0200
              Re: [PATCH 13/15] mtd: nand: samsung: retrieve ECC requirements  from extended ID Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-05-31 00:40 +0200
    [PATCH 14/15] mtd: nand: hynix: rework NAND ID decoding to extract more information Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-05-27 15:00 +0200
    [PATCH 09/15] mtd: nand: move toshiba specific initialization in nand_toshiba.c Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-05-27 15:00 +0200

#1408057 — [PATCH 00/15] mtd: nand: allow vendor specific detection/initialization

Hello,

This patch series is a step forward in supporting vendor-specific
functionalities.
This series is mainly moving vendor-specific initialization or
detection code out of the core, but also introduces an infrastructure
allowing support for vendor-specific features.

While those features might seem useless to most users, some of them are
actually required on modern MLC/TLC NANDs (this is the case of read-retry
support, which AFAICT has not been standardized by the JEDEC consortium).

Now, let's detail what's inside this patch-set.

Patches 1 to 4 are simple reworks simplifying auto-detection function
prototypes, and clarifying their purpose.

Patch 5 is introducing the vendor-specific initialization
infrastructure.

Patch 6 is removing the MTD_NAND_IDS Kconfig option to avoid creating
a nand_ids.ko module when MTD_NAND is enabled as a module. This prevents
a future cross-dependency between nand.ko where all vendor specific
code will rely and nand_ids.ko which will reference vendor-specific ops
in its manufacturer table, which in turn is referenced by the core code
linked in nand.ko.

Patches 7 to 12 are moving vendor-specific code into their respective
nand_<vendor>.c files.

Patch 13 is taking a patch proposed by Hans and adding support for ECC
requirements extraction from the samsung extended IDs. It seems to apply
to all Samsung MLCs, but even if it's not the case, the detection code
should be improved to support the new formats.

Patch 14 is adding support for advanced NAND ID decoding to the Hynix
driver (OOB size, ECC and scrambling requirements extraction). Again
this detection code might be incomplete, but I'd like people to extend
it if required rather than adding new full-id entries in the nand_ids
table.

And finally, patch 15 is showing how useful this vendor-specific stuff
can be by implementing read-retry support for Hynix 1x nm MLCs. And
trust me, you don't want to try using such a NAND without read-retry
support ;).

As always, I'm open to any suggestion to improve this vendor-specific
infrastructure, so please review the code :).

Thanks,

Boris

Boris Brezillon (14):
  mtd: nand: get rid of the mtd parameter in all auto-detection
    functions
  mtd: nand: store nand ID in struct nand_chip
  mtd: nand: get rid of busw parameter
  mtd: nand: rename nand_get_flash_type() into nand_detect()
  mtd: nand: add vendor specific initialization step
  mtd: nand: kill the MTD_NAND_IDS Kconfig option
  mtd: nand: move samsung specific initialization in nand_samsung.c
  mtd: nand: move hynix specific initialization in nand_hynix.c
  mtd: nand: move toshiba specific initialization in nand_toshiba.c
  mtd: nand: move micron specific initialization in nand_micron.c
  mtd: nand: move AMD/Spansion specific initialization in nand_amd.c
  mtd: nand: move Macronix specific initialization in nand_macronix.c
  mtd: nand: hynix: rework NAND ID decoding to extract more information
  mtd: nand: hynix: add read-retry support for 1x nm MLC NANDs

Hans de Goede (1):
  mtd: nand: samsung: retrieve ECC requirements from extended ID

 arch/cris/arch-v32/drivers/Kconfig |   1 -
 drivers/mtd/nand/Kconfig           |   4 -
 drivers/mtd/nand/Makefile          |   9 +-
 drivers/mtd/nand/nand_amd.c        |  60 ++++
 drivers/mtd/nand/nand_base.c       | 362 +++++++--------------
 drivers/mtd/nand/nand_hynix.c      | 623 +++++++++++++++++++++++++++++++++++++
 drivers/mtd/nand/nand_ids.c        |  21 +-
 drivers/mtd/nand/nand_macronix.c   |  27 ++
 drivers/mtd/nand/nand_micron.c     |  84 +++++
 drivers/mtd/nand/nand_samsung.c    | 110 +++++++
 drivers/mtd/nand/nand_toshiba.c    |  60 ++++
 include/linux/mtd/nand.h           |  64 ++--
 12 files changed, 1141 insertions(+), 284 deletions(-)
 create mode 100644 drivers/mtd/nand/nand_amd.c
 create mode 100644 drivers/mtd/nand/nand_hynix.c
 create mode 100644 drivers/mtd/nand/nand_macronix.c
 create mode 100644 drivers/mtd/nand/nand_micron.c
 create mode 100644 drivers/mtd/nand/nand_samsung.c
 create mode 100644 drivers/mtd/nand/nand_toshiba.c

-- 
2.7.4

[toc] | [next] | [standalone]

#1408058 — [PATCH 05/15] mtd: nand: add vendor specific initialization step

A lot of NANDs are implementing generic features in a non-generic way, or
are providing advanced auto-detection logic where the NAND ID bytes meaning
changes with the NAND generation.

Providing this vendor specific initialization step will allow us to get rid
of the full ids in the nand_ids table or all the vendor specific cases
added over the time in the generic NAND ID decoding logic.

Note that nand_decode_bbm_options() call is moved before manuf->ops->init()
because vendor ->init() hook might tweak the BBM flags.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/nand/nand_base.c | 30 ++++++++++++++++++++++++++----
 include/linux/mtd/nand.h     | 31 +++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 95e9a8e..b979e45 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3815,7 +3815,7 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	int busw;
-	int i, maf_idx;
+	int i, maf_idx, ret;
 	u8 *id_data = chip->id.data;
 	u8 maf_id, dev_id;
 
@@ -3925,6 +3925,19 @@ ident_done:
 			break;
 	}
 
+	nand_decode_bbm_options(chip);
+
+	/*
+	 * Vendor specific initialization. This function can ajust the setting
+	 * extracted from generic auto-detection.
+	 */
+	chip->manufacturer.ops = nand_manuf_ids[maf_idx].ops;
+	if (chip->manufacturer.ops && chip->manufacturer.ops->init) {
+		ret = chip->manufacturer.ops->init(chip);
+		if (ret)
+			return ret;
+	}
+
 	if (chip->options & NAND_BUSWIDTH_AUTO) {
 		WARN_ON(busw & NAND_BUSWIDTH_16);
 		nand_set_defaults(chip);
@@ -3938,11 +3951,10 @@ ident_done:
 		pr_info("%s %s\n", nand_manuf_ids[maf_idx].name, mtd->name);
 		pr_warn("bus width %d instead %d bit\n", busw ? 16 : 8,
 			(chip->options & NAND_BUSWIDTH_16) ? 16 : 8);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err_cleanup;
 	}
 
-	nand_decode_bbm_options(chip);
-
 	/* Calculate the address shift from the page size */
 	chip->page_shift = ffs(mtd->writesize) - 1;
 	/* Convert chipsize to number of pages per chip -1 */
@@ -3981,6 +3993,12 @@ ident_done:
 		(int)(chip->chipsize >> 20), nand_is_slc(chip) ? "SLC" : "MLC",
 		mtd->erasesize >> 10, mtd->writesize, mtd->oobsize);
 	return 0;
+
+err_cleanup:
+	if (chip->manufacturer.ops && chip->manufacturer.ops->cleanup)
+		chip->manufacturer.ops->cleanup(chip);
+
+	return ret;
 }
 
 static const char * const nand_ecc_modes[] = {
@@ -4623,6 +4641,10 @@ void nand_release(struct mtd_info *mtd)
 	if (chip->badblock_pattern && chip->badblock_pattern->options
 			& NAND_BBT_DYNAMICSTRUCT)
 		kfree(chip->badblock_pattern);
+
+	/* Release manufacturer private data */
+	if (chip->manufacturer.ops && chip->manufacturer.ops->cleanup)
+		chip->manufacturer.ops->cleanup(chip);
 }
 EXPORT_SYMBOL_GPL(nand_release);
 
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 3072f5e..d8de579 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -577,6 +577,18 @@ struct nand_buffers {
 };
 
 /**
+ * struct nand_manufacturer_ops - NAND Manufacturer operations
+ * @init: initialize all vendor specific fields (like the ->read_retry()
+ *	  implementation) if any.
+ * @cleanup: the ->init() function may have allocated resources, ->cleanup()
+ *	     is here to let vendor specific code release those resources.
+ */
+struct nand_manufacturer_ops {
+	int (*init)(struct nand_chip *chip);
+	void (*cleanup)(struct nand_chip *chip);
+};
+
+/**
  * struct nand_chip - NAND Private Flash Chip Data
  * @mtd:		MTD device registered to the MTD framework
  * @IO_ADDR_R:		[BOARDSPECIFIC] address to read the 8 I/O lines of the
@@ -676,6 +688,7 @@ struct nand_buffers {
  *			additional error status checks (determine if errors are
  *			correctable).
  * @write_page:		[REPLACEABLE] High-level page write function
+ * @manufacturer:	[INTERN] Contains manufacturer data
  */
 
 struct nand_chip {
@@ -756,6 +769,11 @@ struct nand_chip {
 	struct nand_bbt_descr *badblock_pattern;
 
 	void *priv;
+
+	struct {
+		const struct nand_manufacturer_ops *ops;
+		void *priv;
+	} manufacturer;
 };
 
 extern const struct mtd_ooblayout_ops nand_ooblayout_sp_ops;
@@ -792,6 +810,17 @@ static inline void nand_set_controller_data(struct nand_chip *chip, void *priv)
 	chip->priv = priv;
 }
 
+static inline void nand_set_manufacturer_data(struct nand_chip *chip,
+					      void *priv)
+{
+	chip->manufacturer.priv = priv;
+}
+
+static inline void *nand_get_manufacturer_data(struct nand_chip *chip)
+{
+	return chip->manufacturer.priv;
+}
+
 /*
  * NAND Flash Manufacturer ID Codes
  */
@@ -896,10 +925,12 @@ struct nand_flash_dev {
  * struct nand_manufacturers - NAND Flash Manufacturer ID Structure
  * @name:	Manufacturer name
  * @id:		manufacturer ID code of device.
+ * @ops:	manufacturer operations
 */
 struct nand_manufacturers {
 	int id;
 	char *name;
+	const struct nand_manufacturer_ops *ops;
 };
 
 extern struct nand_flash_dev nand_flash_ids[];
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1408059 — [PATCH 13/15] mtd: nand: samsung: retrieve ECC requirements from extended ID

From: Hans de Goede <hdegoede@redhat.com>

On some nand controllers with hw-ecc the controller code wants to know
the ecc strength and size and having these as 0, 0 is not accepted.

Specifying these in devicetree is possible but undesirable as the nand
may be different in different production runs of the same board, so it
is better to get this info from the nand id where possible.

This commit adds code to read the ecc strength and size from the nand
for Samsung extended-id nands. This code is based on the info for the 5th
id byte in the datasheets for the following Samsung nands: K9GAG08U0E,
K9GAG08U0F, K9GAG08X0D, K9GBG08U0A, K9GBG08U0B. These all use these bits
in the exact same way.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/nand/nand_samsung.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/mtd/nand/nand_samsung.c b/drivers/mtd/nand/nand_samsung.c
index 27ede41..39448ff 100644
--- a/drivers/mtd/nand/nand_samsung.c
+++ b/drivers/mtd/nand/nand_samsung.c
@@ -61,6 +61,26 @@ static int samsung_nand_decode_id(struct nand_chip *chip)
 		extid >>= 2;
 		mtd->erasesize = (128 * 1024) <<
 				 (((extid >> 1) & 0x04) | (extid & 0x03));
+
+		/* Extract ECC requirements from 5th id byte*/
+		extid = (chip->id.data[4] >> 4) & 0x07;
+		if (extid < 5) {
+			chip->ecc_step_ds = 512;
+			chip->ecc_strength_ds = 1 << extid;
+		} else {
+			chip->ecc_step_ds = 1024;
+			switch (extid) {
+			case 5:
+				chip->ecc_strength_ds = 24;
+				break;
+			case 6:
+				chip->ecc_strength_ds = 40;
+				break;
+			case 7:
+				chip->ecc_strength_ds = 60;
+				break;
+			}
+		}
 	}
 
 	if (mtd->writesize > 512)
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1408688 — Re: [PATCH 13/15] mtd: nand: samsung: retrieve ECC requirements from extended ID

[Multipart message — attachments visible in raw view] — view raw

On Fri, 27 May 2016 14:54:59 +0200, Boris Brezillon said:
> From: Hans de Goede <hdegoede@redhat.com>
>
> On some nand controllers with hw-ecc the controller code wants to know
> the ecc strength and size and having these as 0, 0 is not accepted.
>
> Specifying these in devicetree is possible but undesirable as the nand
> may be different in different production runs of the same board, so it
> is better to get this info from the nand id where possible.
>
> This commit adds code to read the ecc strength and size from the nand
> for Samsung extended-id nands. This code is based on the info for the 5th
> id byte in the datasheets for the following Samsung nands: K9GAG08U0E,
> K9GAG08U0F, K9GAG08X0D, K9GBG08U0A, K9GBG08U0B. These all use these bits
> in the exact same way.

Is this correct for all Samsung nand devices supported by this driver?

(If this driver only covers those 5 specific parts, it's OK.  If there's
others, more research is needed....)

[toc] | [prev] | [next] | [standalone]

#1408827 — Re: [PATCH 13/15] mtd: nand: samsung: retrieve ECC requirements from extended ID

Hi Valdis,

On Sun, 29 May 2016 20:20:35 -0400
Valdis.Kletnieks@vt.edu wrote:

> On Fri, 27 May 2016 14:54:59 +0200, Boris Brezillon said:
> > From: Hans de Goede <hdegoede@redhat.com>
> >
> > On some nand controllers with hw-ecc the controller code wants to know
> > the ecc strength and size and having these as 0, 0 is not accepted.
> >
> > Specifying these in devicetree is possible but undesirable as the nand
> > may be different in different production runs of the same board, so it
> > is better to get this info from the nand id where possible.
> >
> > This commit adds code to read the ecc strength and size from the nand
> > for Samsung extended-id nands. This code is based on the info for the 5th
> > id byte in the datasheets for the following Samsung nands: K9GAG08U0E,
> > K9GAG08U0F, K9GAG08X0D, K9GBG08U0A, K9GBG08U0B. These all use these bits
> > in the exact same way.  
> 
> Is this correct for all Samsung nand devices supported by this driver?
> 
> (If this driver only covers those 5 specific parts, it's OK.  If there's
> others, more research is needed....)

Actually, that was my first reaction [1], but the more I think about it
the more I realize it's a non-issue.
AFAICT, there's no full-id entries for Samsung NANDs in the nand_ids
table, so this either means there's no real users of Samsung MLCs or
NAND controller drivers connecting to those chips don't care about the
->ecc_{step_ds,strength_ds} fields.

I agree that the solution is not perfect, but I'd prefer seeing the
NAND detection code iteratively improved than rejecting everything
until we're 100% sure that all cases are correctly handled (which might
never happen since NAND vendors introduce new NAND ID scheme if they
need to).

BTW, do you have Samsung datasheets describing a different NAND ID
format, or is it purely hypothetical?

Regards,

Boris

[1]http://lists.infradead.org/pipermail/linux-mtd/2015-July/060582.html

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1409406 — Re: [PATCH 13/15] mtd: nand: samsung: retrieve ECC requirements from extended ID

[Multipart message — attachments visible in raw view] — view raw

On Mon, 30 May 2016 09:44:46 +0200, Boris Brezillon said:
> Hi Valdis,

> Actually, that was my first reaction [1], but the more I think about it
> the more I realize it's a non-issue.
> AFAICT, there's no full-id entries for Samsung NANDs in the nand_ids
> table, so this either means there's no real users of Samsung MLCs or
> NAND controller drivers connecting to those chips don't care about the
> ->ecc_{step_ds,strength_ds} fields.

I'm mostly, though not totally convinced (not having looked closely at
the existing code).  There's still a possible issue with the distinction
between:

A) "driver never references the variable" and

B) driver check if it's zero, and acts like it doesn't care if it is, but if
it's non-zero, it goes ahead and uses it, with possible hilarity ensuing if the
value is wrong.

Should be pretty easy for somebody who knows the code better than I to rule
out case B fairly quickly...

> I agree that the solution is not perfect, but I'd prefer seeing the
> NAND detection code iteratively improved than rejecting everything
> until we're 100% sure that all cases are correctly handled (which might
> never happen since NAND vendors introduce new NAND ID scheme if they
> need to).
>
> BTW, do you have Samsung datasheets describing a different NAND ID
> format, or is it purely hypothetical?

Mostly hypothetical.  I've just seen too many patches that assume "all chips
from  vendor XYZ do *this*" that were not at all corrrect.

[toc] | [prev] | [next] | [standalone]

#1409706 — Re: [PATCH 13/15] mtd: nand: samsung: retrieve ECC requirements from extended ID

On Mon, 30 May 2016 16:56:09 -0400
Valdis.Kletnieks@vt.edu wrote:

> On Mon, 30 May 2016 09:44:46 +0200, Boris Brezillon said:
> > Hi Valdis,  
> 
> > Actually, that was my first reaction [1], but the more I think about it
> > the more I realize it's a non-issue.
> > AFAICT, there's no full-id entries for Samsung NANDs in the nand_ids
> > table, so this either means there's no real users of Samsung MLCs or
> > NAND controller drivers connecting to those chips don't care about the  
> > ->ecc_{step_ds,strength_ds} fields.  
> 
> I'm mostly, though not totally convinced (not having looked closely at
> the existing code).  There's still a possible issue with the distinction
> between:
> 
> A) "driver never references the variable" and
> 
> B) driver check if it's zero, and acts like it doesn't care if it is, but if
> it's non-zero, it goes ahead and uses it, with possible hilarity ensuing if the
> value is wrong.
> 
> Should be pretty easy for somebody who knows the code better than I to rule
> out case B fairly quickly...

Ok, so I had a quick look, and only 4 drivers are actually using the
->ecc_{strength,step}_ds fields, and AFAICT, all of them are already
broken with the existing implementation, even if those fields are set
to 0.

- the atmel driver uses a default ECC config (2bits/512bytes) if
  those fields are set to 0, and this config is clearly not suitable
  for the MLC NANDs we are talking about (note that SLC NANDs seem to
  all use the 4 bytes extended ID scheme, which seems to be common to
  all vendors).

- the gpmi driver either returns an error if one of these fields
  are set to zero and the 'fsl,use-minimum-ecc' DT property is defined,
  or tries to fill the whole OOB area with ECC bytes if the property is
  not defined. The 2nd solution could work, if only we were sure about
  the encoding of the OOB size, but, as the ECC requirements field, it
  depends on the extended ID scheme. So, in the end, it's broken too.

- the pxa and sunxi drivers are just blindly relying on those fields if
  the 'nand-ecc-strength' and 'nand-ecc-step-size' properties are
  undefined. The pxa default to 1bit/512bytes if ecc strength or ecc
  step appear to be set to 0, while the sunxi driver completely rejects
  the NAND chip.
  In both cases, the current implementation is broken, either because
  you will use an unsuitable ECC config or because your NAND chip won't
  be registered.

So, as you can see, we're just moving from a broken state to another
broken state, except the new infrastructures allows one to extend the
detection logic and thus allow for correct detection of more chips.

> 
> > I agree that the solution is not perfect, but I'd prefer seeing the
> > NAND detection code iteratively improved than rejecting everything
> > until we're 100% sure that all cases are correctly handled (which might
> > never happen since NAND vendors introduce new NAND ID scheme if they
> > need to).
> >
> > BTW, do you have Samsung datasheets describing a different NAND ID
> > format, or is it purely hypothetical?  
> 
> Mostly hypothetical.  I've just seen too many patches that assume "all chips
> from  vendor XYZ do *this*" that were not at all corrrect.
> 

Yep, that's true, except I'm not promising anything here, I just say
that this patch adds code to detect a range of Samsung chips, and that
it can be extended to properly detect chips that do not use this format
if we appear to find some (which is very likely to happen).

Of course, we could decide to leave everything as is and add full-id
entries to the nand_ids table each time we want to support a new chip
that does not expose a valid ONFI of JEDEC parameter table. But that
means adding more and more info to the nand_flash_dev structure and
polluting the nand_ids table with a bunch of NAND chips that could
otherwise be handled by the same detection code.
And as detailed above, this solution is just as broken as mine but in a
different way (in both cases, NANDs that are not already supported by
the kernel will either be rejected or used ).

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1409742 — Re: [PATCH 13/15] mtd: nand: samsung: retrieve ECC requirements from extended ID

On Tue, 31 May 2016 00:28:24 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Mon, 30 May 2016 16:56:09 -0400
> Valdis.Kletnieks@vt.edu wrote:
> 
> > On Mon, 30 May 2016 09:44:46 +0200, Boris Brezillon said:  
> > > Hi Valdis,    
> >   
> > > Actually, that was my first reaction [1], but the more I think about it
> > > the more I realize it's a non-issue.
> > > AFAICT, there's no full-id entries for Samsung NANDs in the nand_ids
> > > table, so this either means there's no real users of Samsung MLCs or
> > > NAND controller drivers connecting to those chips don't care about the    
> > > ->ecc_{step_ds,strength_ds} fields.    
> > 
> > I'm mostly, though not totally convinced (not having looked closely at
> > the existing code).  There's still a possible issue with the distinction
> > between:
> > 
> > A) "driver never references the variable" and
> > 
> > B) driver check if it's zero, and acts like it doesn't care if it is, but if
> > it's non-zero, it goes ahead and uses it, with possible hilarity ensuing if the
> > value is wrong.
> > 
> > Should be pretty easy for somebody who knows the code better than I to rule
> > out case B fairly quickly...  
> 
> Ok, so I had a quick look, and only 4 drivers are actually using the
> ->ecc_{strength,step}_ds fields, and AFAICT, all of them are already  
> broken with the existing implementation, even if those fields are set
> to 0.
> 
> - the atmel driver uses a default ECC config (2bits/512bytes) if
>   those fields are set to 0, and this config is clearly not suitable
>   for the MLC NANDs we are talking about (note that SLC NANDs seem to
>   all use the 4 bytes extended ID scheme, which seems to be common to
>   all vendors).
> 
> - the gpmi driver either returns an error if one of these fields
>   are set to zero and the 'fsl,use-minimum-ecc' DT property is defined,
>   or tries to fill the whole OOB area with ECC bytes if the property is
>   not defined. The 2nd solution could work, if only we were sure about
>   the encoding of the OOB size, but, as the ECC requirements field, it
>   depends on the extended ID scheme. So, in the end, it's broken too.
> 
> - the pxa and sunxi drivers are just blindly relying on those fields if
>   the 'nand-ecc-strength' and 'nand-ecc-step-size' properties are
>   undefined. The pxa default to 1bit/512bytes if ecc strength or ecc
>   step appear to be set to 0, while the sunxi driver completely rejects
>   the NAND chip.
>   In both cases, the current implementation is broken, either because
>   you will use an unsuitable ECC config or because your NAND chip won't
>   be registered.
> 
> So, as you can see, we're just moving from a broken state to another
> broken state, except the new infrastructures allows one to extend the
> detection logic and thus allow for correct detection of more chips.
> 
> >   
> > > I agree that the solution is not perfect, but I'd prefer seeing the
> > > NAND detection code iteratively improved than rejecting everything
> > > until we're 100% sure that all cases are correctly handled (which might
> > > never happen since NAND vendors introduce new NAND ID scheme if they
> > > need to).
> > >
> > > BTW, do you have Samsung datasheets describing a different NAND ID
> > > format, or is it purely hypothetical?    
> > 
> > Mostly hypothetical.  I've just seen too many patches that assume "all chips
> > from  vendor XYZ do *this*" that were not at all corrrect.
> >   
> 
> Yep, that's true, except I'm not promising anything here, I just say
> that this patch adds code to detect a range of Samsung chips, and that
> it can be extended to properly detect chips that do not use this format
> if we appear to find some (which is very likely to happen).
> 
> Of course, we could decide to leave everything as is and add full-id
> entries to the nand_ids table each time we want to support a new chip
> that does not expose a valid ONFI of JEDEC parameter table. But that
> means adding more and more info to the nand_flash_dev structure and
> polluting the nand_ids table with a bunch of NAND chips that could
> otherwise be handled by the same detection code.
> And as detailed above, this solution is just as broken as mine but in a
> different way (in both cases, NANDs that are not already supported by
> the kernel will either be rejected or used ).
                                            ^ inappropriately 



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1408061 — [PATCH 14/15] mtd: nand: hynix: rework NAND ID decoding to extract more information

The current NAND ID detection in nand_hynix.c is not handling the
different scheme used by Hynix, thus forcing developers to add new
entries in the nand_ids table each time they want to support a new MLC
NAND.

Enhance the detection logic to handle all known formats. This does not
necessarily mean we are handling all the cases, but if new formats are
discovered, the code should evolve to take them into account instead of
adding more full-id entries in the nand_ids table.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/nand/nand_hynix.c | 226 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 202 insertions(+), 24 deletions(-)

diff --git a/drivers/mtd/nand/nand_hynix.c b/drivers/mtd/nand/nand_hynix.c
index 3a4e1a4..cad1d99 100644
--- a/drivers/mtd/nand/nand_hynix.c
+++ b/drivers/mtd/nand/nand_hynix.c
@@ -14,26 +14,47 @@
 
 #include <linux/mtd/nand.h>
 
-static int hynix_nand_decode_id(struct nand_chip *chip)
+static bool hynix_nand_has_valid_jedecid(struct nand_chip *chip)
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
+	u8 jedecid[6] = { };
+	int i = 0;
 
-	/*
-	 * The NAND is compatible with the JEDEC or ONFI standard, we already
-	 * have all the information we could retrieve from the extended ID.
-	 */
-	if (chip->onfi_version || chip->jedec_version)
-		return 0;
+	chip->cmdfunc(mtd, NAND_CMD_READID, 0x40, -1);
+	for (i = 0; i < 5; i++)
+		jedecid[i] = chip->read_byte(mtd);
+
+	return !strcmp("JEDEC", jedecid);
+}
 
-	/* Hynix MLC   (6 byte ID): Hynix H27UBG8T2B (p.22) */
-	if (chip->id.len == 6 && !nand_is_slc(chip)) {
-		u8 tmp, extid = chip->id.data[3] >> 2;
+static int hynix_nand_extract_oobsize(struct nand_chip *chip,
+				      bool valid_jedecid)
+{
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	u8 oobsize;
 
-		/* Adjust pagesize */
-		mtd->writesize <<= 1;
+	oobsize = ((chip->id.data[3] >> 2) & 0x3) |
+		  ((chip->id.data[3] >> 4) & 0x4);
 
-		/* Fix oobsize */
-		switch (((extid >> 2) & 0x4) | (extid & 0x3)) {
+	if (valid_jedecid) {
+		switch (oobsize) {
+		case 0:
+			mtd->oobsize = 2048;
+			break;
+		case 1:
+			mtd->oobsize = 1664;
+			break;
+		case 2:
+			mtd->oobsize = 1024;
+			break;
+		case 3:
+			mtd->oobsize = 640;
+			break;
+		default:
+			return -EINVAL;
+		}
+	} else {
+		switch (oobsize) {
 		case 0:
 			mtd->oobsize = 128;
 			break;
@@ -52,21 +73,178 @@ static int hynix_nand_decode_id(struct nand_chip *chip)
 		case 5:
 			mtd->oobsize = 16;
 			break;
-		default:
+		case 6:
 			mtd->oobsize = 640;
 			break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+static int hynix_nand_extract_ecc_requirements(struct nand_chip *chip,
+					       bool valid_jedecid)
+{
+	u8 ecc_level = (chip->id.data[4] >> 4) & 0x7;
+
+	if (valid_jedecid) {
+		/* Reference: H27UCG8T2E datasheet */
+		chip->ecc_step_ds = 1024;
+
+		switch (ecc_level) {
+		case 0:
+			chip->ecc_step_ds = 0;
+			chip->ecc_strength_ds = 0;
+			break;
+		case 1:
+			chip->ecc_strength_ds = 4;
+			break;
+		case 2:
+			chip->ecc_strength_ds = 24;
+			break;
+		case 3:
+			chip->ecc_strength_ds = 32;
+			break;
+		case 4:
+			chip->ecc_strength_ds = 40;
+			break;
+		case 5:
+			chip->ecc_strength_ds = 50;
+			break;
+		case 6:
+			chip->ecc_strength_ds = 60;
+			break;
+		default:
+			return -EINVAL;
+		}
+	} else {
+		/*
+		 * The ECC requirements field meaning depends on the
+		 * NAND technology.
+		 */
+		u8 nand_tech = chip->id.data[5] & 0x3;
+
+		if (nand_tech < 3) {
+			/* > 26nm, reference: H27UBG8T2A datasheet */
+			if (ecc_level < 5) {
+				chip->ecc_step_ds = 512;
+				chip->ecc_strength_ds = 1 << ecc_level;
+			} else if (ecc_level < 7) {
+				if (ecc_level == 5)
+					chip->ecc_step_ds = 2048;
+				else
+					chip->ecc_step_ds = 2048;
+				chip->ecc_strength_ds = 24;
+			} else {
+				return -EINVAL;
+			}
+		} else {
+			/* <= 26nm, reference: H27UBG8T2B datasheet */
+			if (!ecc_level) {
+				chip->ecc_step_ds = 0;
+				chip->ecc_strength_ds = 0;
+			} else if (ecc_level < 5) {
+				chip->ecc_step_ds = 512;
+				chip->ecc_strength_ds = 1 << (ecc_level - 1);
+			} else {
+				chip->ecc_step_ds = 1024;
+				chip->ecc_strength_ds = 24 +
+							(8 * (ecc_level - 5));
+			}
 		}
+	}
+
+	return 0;
+}
+
+static void hynix_nand_extract_scrambling_requirements(struct nand_chip *chip,
+						       bool valid_jedecid)
+{
+	u8 nand_tech;
+
+	/* We need scrambling on all TLC NANDs*/
+	if (chip->bits_per_cell > 2)
+		chip->options |= NAND_NEED_SCRAMBLING;
+
+	/* And on MLC NANDs with sub-3xnm process */
+	if (valid_jedecid) {
+		nand_tech = chip->id.data[5] >> 4;
+
+		/* < 3xnm */
+		if (nand_tech > 0)
+			chip->options |= NAND_NEED_SCRAMBLING;
+	} else {
+		nand_tech = chip->id.data[5] & 0x3;
 
-		/* Fix blocksize */
-		extid >>= 2;
-		tmp = ((extid >> 1) & 0x04) | (extid & 0x03);
-		if (tmp < 0x03)
-			mtd->erasesize = (128 * 1024) << tmp;
-		else if (tmp == 0x03)
-			mtd->erasesize = 768 * 1024;
-		else
-			mtd->erasesize = (64 * 1024) << tmp;
+		/* < 32nm */
+		if (nand_tech > 2)
+			chip->options |= NAND_NEED_SCRAMBLING;
 	}
+}
+
+static int hynix_nand_decode_id(struct nand_chip *chip)
+{
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	bool valid_jedecid;
+	int ret;
+	u8 tmp;
+
+	/*
+	 * The NAND is compatible with the JEDEC or ONFI standard, we already
+	 * have all the information we could retrieve from the extended ID.
+	 */
+	if (chip->onfi_version || chip->jedec_version)
+		return 0;
+
+	/*
+	 * Exclude all SLC NANDs from this advanced detection scheme.
+	 * According to the ranges defined in several datasheets, it might
+	 * appear that even SLC NANDs could fall in this extended ID scheme.
+	 * If that the case rework the test to let SLC NANDs go through the
+	 * detection process.
+	 */
+	if (chip->id.len < 6 || nand_is_slc(chip))
+		return 0;
+
+	/*
+	 * Adjust pagesize. On MLC NANDs, we start at 128KiB not 64KiB,
+	 * a simple left shift does the trick.
+	 */
+	mtd->writesize <<= 1;
+
+	tmp = (chip->id.data[3] >> 4) & 0x3;
+	/*
+	 * When bit7 is set that means we start counting at 1MiB, otherwise
+	 * we start counting at 128KiB and shift this value the content of
+	 * ID[3][4:5].
+	 * The only exception is when ID[3][4:5] == 3 and ID[3][7] == 0, in
+	 * this case the erasesize is set to 768KiB.
+	 */
+	if (chip->id.data[3] & 0x80)
+		mtd->erasesize = SZ_1M << tmp;
+	else if (tmp == 3)
+		mtd->erasesize = SZ_512K + SZ_256K;
+	else
+		mtd->erasesize = SZ_128K << tmp;
+
+	/*
+	 * Modern Toggle DDR NANDs have a valid JEDECID even though they are
+	 * not exposing a valid JEDEC parameter table.
+	 * These NANDs use a different NAND ID scheme.
+	 */
+	valid_jedecid = hynix_nand_has_valid_jedecid(chip);
+
+	ret = hynix_nand_extract_oobsize(chip, valid_jedecid);
+	if (ret)
+		return ret;
+
+	ret = hynix_nand_extract_ecc_requirements(chip, valid_jedecid);
+	if (ret)
+		return ret;
+
+	hynix_nand_extract_scrambling_requirements(chip, valid_jedecid);
 
 	return 0;
 }
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1408062 — [PATCH 09/15] mtd: nand: move toshiba specific initialization in nand_toshiba.c

Moving Hynix specific initialization into nand_toshiba.c. This is part
of the "separate vendor specific code from core" cleanup process.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/nand/Makefile       |  1 +
 drivers/mtd/nand/nand_base.c    | 19 ++-----------
 drivers/mtd/nand/nand_ids.c     |  3 ++-
 drivers/mtd/nand/nand_toshiba.c | 60 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 65 insertions(+), 18 deletions(-)
 create mode 100644 drivers/mtd/nand/nand_toshiba.c

diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index d79338f..403e9a7 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -60,3 +60,4 @@ obj-$(CONFIG_MTD_NAND_QCOM)		+= qcom_nandc.o
 nand-objs := nand_base.o nand_bbt.o nand_timings.o nand_ids.o
 nand-objs += nand_hynix.o
 nand-objs += nand_samsung.o
+nand-objs += nand_toshiba.o
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index ce0ea01..38cc839 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3583,7 +3583,7 @@ static int nand_get_bits_per_cell(u8 cellinfo)
 static void nand_decode_ext_id(struct nand_chip *chip)
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
-	int extid, id_len = chip->id.len;
+	int extid;
 	u8 *id_data = chip->id.data;
 	/* The 3rd id byte holds MLC / multichip data */
 	chip->bits_per_cell = nand_get_bits_per_cell(id_data[2]);
@@ -3602,20 +3602,6 @@ static void nand_decode_ext_id(struct nand_chip *chip)
 	/* Get buswidth information */
 	if (extid & 0x1)
 		chip->options |= NAND_BUSWIDTH_16;
-
-	/*
-	 * Toshiba 24nm raw SLC (i.e., not BENAND) have 32B OOB per
-	 * 512B page. For Toshiba SLC, we decode the 5th/6th byte as
-	 * follows:
-	 * - ID byte 6, bits[2:0]: 100b -> 43nm, 101b -> 32nm,
-	 *                         110b -> 24nm
-	 * - ID byte 5, bit[7]:    1 -> BENAND, 0 -> raw SLC
-	 */
-	if (id_len >= 6 && id_data[0] == NAND_MFR_TOSHIBA &&
-	    nand_is_slc(chip) &&
-	    (id_data[5] & 0x7) == 0x6 /* 24nm */ &&
-	     !(id_data[4] & 0x80) /* !BENAND */)
-		mtd->oobsize = 32 * mtd->writesize >> 9;
 }
 
 /*
@@ -3674,8 +3660,7 @@ static void nand_decode_bbm_options(struct nand_chip *chip)
 	 * AMD/Spansion, and Macronix.  All others scan only the first page.
 	 */
 	if ((nand_is_slc(chip) &&
-				(maf_id == NAND_MFR_TOSHIBA ||
-				 maf_id == NAND_MFR_AMD ||
+				(maf_id == NAND_MFR_AMD ||
 				 maf_id == NAND_MFR_MACRONIX)) ||
 			(mtd->writesize == 2048 &&
 			 maf_id == NAND_MFR_MICRON))
diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c
index 7bf93b8..2cc9d32 100644
--- a/drivers/mtd/nand/nand_ids.c
+++ b/drivers/mtd/nand/nand_ids.c
@@ -166,11 +166,12 @@ struct nand_flash_dev nand_flash_ids[] = {
 };
 
 /* Manufacturer IDs */
+extern const struct nand_manufacturer_ops toshiba_nand_manuf_ops;
 extern const struct nand_manufacturer_ops samsung_nand_manuf_ops;
 extern const struct nand_manufacturer_ops hynix_nand_manuf_ops;
 
 struct nand_manufacturers nand_manuf_ids[] = {
-	{NAND_MFR_TOSHIBA, "Toshiba"},
+	{NAND_MFR_TOSHIBA, "Toshiba", &toshiba_nand_manuf_ops},
 	{NAND_MFR_SAMSUNG, "Samsung", &samsung_nand_manuf_ops},
 	{NAND_MFR_FUJITSU, "Fujitsu"},
 	{NAND_MFR_NATIONAL, "National"},
diff --git a/drivers/mtd/nand/nand_toshiba.c b/drivers/mtd/nand/nand_toshiba.c
new file mode 100644
index 0000000..0d6b319
--- /dev/null
+++ b/drivers/mtd/nand/nand_toshiba.c
@@ -0,0 +1,60 @@
+/*
+ * Copyright (C) 2013 Boris Brezillon <boris.brezillon@free-electrons.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/mtd/nand.h>
+
+static int toshiba_nand_decode_id(struct nand_chip *chip)
+{
+	struct mtd_info *mtd = nand_to_mtd(chip);
+
+	/*
+	 * The NAND is compatible with the JEDEC or ONFI standard, we already
+	 * have all the information we could retrieve from the extended ID.
+	 */
+	if (chip->onfi_version || chip->jedec_version)
+		return 0;
+
+	/*
+	 * Toshiba 24nm raw SLC (i.e., not BENAND) have 32B OOB per
+	 * 512B page. For Toshiba SLC, we decode the 5th/6th byte as
+	 * follows:
+	 * - ID byte 6, bits[2:0]: 100b -> 43nm, 101b -> 32nm,
+	 *                         110b -> 24nm
+	 * - ID byte 5, bit[7]:    1 -> BENAND, 0 -> raw SLC
+	 */
+	if (chip->id.len >= 6 && nand_is_slc(chip) &&
+	    (chip->id.data[5] & 0x7) == 0x6 /* 24nm */ &&
+	    !(chip->id.data[4] & 0x80) /* !BENAND */)
+		mtd->oobsize = 32 * mtd->writesize >> 9;
+
+	return 0;
+}
+
+static int toshiba_nand_init(struct nand_chip *chip)
+{
+	int ret;
+
+	ret = toshiba_nand_decode_id(chip);
+	if (ret)
+		return ret;
+
+	if (nand_is_slc(chip))
+		chip->bbt_options |= NAND_BBT_SCAN2NDPAGE;
+
+	return 0;
+}
+
+const struct nand_manufacturer_ops toshiba_nand_manuf_ops = {
+	.init = toshiba_nand_init,
+};
-- 
2.7.4

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web