Re: [PATCH v6 16/17] mtd: nand: hynix: Rework NAND ID decoding to extract more information

Re: [PATCH v6 16/17] mtd: nand: hynix: Rework NAND ID decoding to extract more information

[Boris Brezillon]

Hi Julia,

On Mon, 9 Jan 2017 21:21:27 +0100 (CET)
Julia Lawall <julia.lawall@lip6.fr> wrote:

> It looks odd that lines 158 and 160 are the same.

Indeed, it should be 'chip->ecc_step_ds = 1024;' in the else branch.
I'll fix that.

Thanks,

Boris

> 
> julia
> 
> ---------- Forwarded message ----------
> 
> In-Reply-To: <1483956264-3335-17-git-send-email-boris.brezillon@free-electrons.com>
> 
> Hi Boris,
> 
> [auto build test WARNING on mtd/master]
> [also build test WARNING on v4.10-rc3 next-20170106]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Boris-Brezillon/mtd-nand-allow-vendor-specific-detection-initialization/20170110-022221
> base:   git://git.infradead.org/linux-mtd.git master
> :::::: branch date: 2 hours ago
> :::::: commit date: 2 hours ago
> 
> >> drivers/mtd/nand/nand_hynix.c:157:4-6: WARNING: possible condition with no effect (if == else)  
> 
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout ee58e9ecc231f4a40ff46fe95078d7796ebe430b
> vim +157 drivers/mtd/nand/nand_hynix.c
> 
> ee58e9ec Boris Brezillon 2017-01-09  141  			 */
> ee58e9ec Boris Brezillon 2017-01-09  142  			WARN(1, "Invalid ECC requirements");
> 1065fa22 Boris Brezillon 2017-01-09  143  		}
> ee58e9ec Boris Brezillon 2017-01-09  144  	} else {
> ee58e9ec Boris Brezillon 2017-01-09  145  		/*
> ee58e9ec Boris Brezillon 2017-01-09  146  		 * The ECC requirements field meaning depends on the
> ee58e9ec Boris Brezillon 2017-01-09  147  		 * NAND technology.
> ee58e9ec Boris Brezillon 2017-01-09  148  		 */
> ee58e9ec Boris Brezillon 2017-01-09  149  		u8 nand_tech = chip->id.data[5] & 0x3;
> 1065fa22 Boris Brezillon 2017-01-09  150
> ee58e9ec Boris Brezillon 2017-01-09  151  		if (nand_tech < 3) {
> ee58e9ec Boris Brezillon 2017-01-09  152  			/* > 26nm, reference: H27UBG8T2A datasheet */
> ee58e9ec Boris Brezillon 2017-01-09  153  			if (ecc_level < 5) {
> ee58e9ec Boris Brezillon 2017-01-09  154  				chip->ecc_step_ds = 512;
> ee58e9ec Boris Brezillon 2017-01-09  155  				chip->ecc_strength_ds = 1 << ecc_level;
> ee58e9ec Boris Brezillon 2017-01-09  156  			} else if (ecc_level < 7) {
> ee58e9ec Boris Brezillon 2017-01-09 @157  				if (ecc_level == 5)
> ee58e9ec Boris Brezillon 2017-01-09  158  					chip->ecc_step_ds = 2048;
> 1065fa22 Boris Brezillon 2017-01-09  159  				else
> ee58e9ec Boris Brezillon 2017-01-09  160  					chip->ecc_step_ds = 2048;
> ee58e9ec Boris Brezillon 2017-01-09  161  				chip->ecc_strength_ds = 24;
> ee58e9ec Boris Brezillon 2017-01-09  162  			} else {
> ee58e9ec Boris Brezillon 2017-01-09  163  				/*
> ee58e9ec Boris Brezillon 2017-01-09  164  				 * We should never reach this case, but if that
> ee58e9ec Boris Brezillon 2017-01-09  165  				 * happens, this probably means Hynix decided
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

[Boris Brezillon]

Hello,

I know I said it would be the last round, but I decided to change a few
things after Marek's review.

Marek, Richard, I dropped your R-b/A-b tags on "mtd: nand: Add
manufacturer specific initialization/detection steps" (patch 8) and
"mtd: nand: Kill the MTD_NAND_IDS Kconfig option" (patch 6) since they
changed a bit in v6.
Can you have a look?

Marek, I did not address your concern regarding the use of the extern
keyword for struct nand_manufacturer_ops defs. Let me know if this is
something you really find important, and why. If you have a good reason
to hide these objects behind an accessor, I might reconsider your
suggestion.

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 5 are simple reworks simplifying auto-detection function
prototypes, and clarifying their purpose.

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.

Patch 7 is hiding NAND manufacturer table internals and exposing a
helper to get a nand_manufacturer object from a manufacturer ID.

Patch 8 is introducing the vendor-specific initialization
infrastructure.

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

Patch 15 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 16 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 17 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

Changes since v5:
- Move extern extern struct nand_manufacturer_ops definitions to nand.h
  to fix checkpatch warnings
- Remove MODULE_XX() and EXPORT_SYMBOL() calls in nand_ids.c
- Do not expose the NAND manufacturer table
- Drop the 's' in struct nand_manufacturer*s*
- Provide a pointer to a nand_manufacturer object instead of
  nand_manufacturer_ops in struct nand_chip

Changes since v4:
- Fix copyright headers in nand_<vendor>.c files
- Fix comments referring to Samsung in nand_hynix.c
- Fix commit message of patch 4

Changes since v3:
- Fix coding style issues 

Changes since v2:
- Fix nand_command_lp() implementation to allow reading the ID
  after switching from ->cmdfunc() from nand_command() to
  nand_command_lp().
- Include slab.h in hynix_nand.c
- Avoid selecting/unselecting the NAND chip in Hynix ->init()
  hook (the chip is already selected by the core)
- Add wrappers to call the ->detect() and ->init() hooks

Changes since v1:
- split detection and initialization steps to avoid keeping
  information retrieved by nand_decode_ext_id() if it's not
  appropriate (Aleksei reported a bug where NAND_BUSWIDTH_16
  was set by nand_decode_ext_id() and not cleared by the
  samsung ->init() function).
  The new approach is to call ->detect() if it's provided and
  fallback to nand_decode_ext_id() if it's not. ->detect()
  implementation should can call nand_decode_ext_id() if needed.

Boris Brezillon (16):
  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: Rename the nand_manufacturers struct
  mtd: nand: Kill the MTD_NAND_IDS Kconfig option
  mtd: nand: Do not expose the NAND manufacturer table directly
  mtd: nand: Add manufacturer specific initialization/detection steps
  mtd: nand: Move Samsung specific init/detection logic in
    nand_samsung.c
  mtd: nand: Move Hynix specific init/detection logic in nand_hynix.c
  mtd: nand: Move Toshiba specific init/detection logic in
    nand_toshiba.c
  mtd: nand: Move Micron specific init logic in nand_micron.c
  mtd: nand: Move AMD/Spansion specific init/detection logic 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        |  51 +++
 drivers/mtd/nand/nand_base.c       | 408 +++++++++---------------
 drivers/mtd/nand/nand_hynix.c      | 629 +++++++++++++++++++++++++++++++++++++
 drivers/mtd/nand/nand_ids.c        |  39 ++-
 drivers/mtd/nand/nand_macronix.c   |  30 ++
 drivers/mtd/nand/nand_micron.c     |  86 +++++
 drivers/mtd/nand/nand_samsung.c    | 112 +++++++
 drivers/mtd/nand/nand_toshiba.c    |  51 +++
 include/linux/mtd/nand.h           |  88 ++++--
 12 files changed, 1209 insertions(+), 299 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

[PATCH v6 16/17] mtd: nand: hynix: Rework NAND ID decoding to extract more information

[Boris Brezillon]

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>
Acked-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/nand/nand_hynix.c | 228 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 209 insertions(+), 19 deletions(-)

diff --git a/drivers/mtd/nand/nand_hynix.c b/drivers/mtd/nand/nand_hynix.c
index fbd661688158..92b8e435776c 100644
--- a/drivers/mtd/nand/nand_hynix.c
+++ b/drivers/mtd/nand/nand_hynix.c
@@ -16,21 +16,56 @@
  */
 
 #include <linux/mtd/nand.h>
+#include <linux/sizes.h>
 
-static void 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;
+
+	chip->cmdfunc(mtd, NAND_CMD_READID, 0x40, -1);
+	for (i = 0; i < 5; i++)
+		jedecid[i] = chip->read_byte(mtd);
 
-	/* 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];
+	return !strcmp("JEDEC", jedecid);
+}
 
-		/* Extract pagesize */
-		mtd->writesize = 2048 << (extid & 0x03);
-		extid >>= 2;
+static void hynix_nand_extract_oobsize(struct nand_chip *chip,
+				       bool valid_jedecid)
+{
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	u8 oobsize;
 
-		/* Extract oobsize */
-		switch (((extid >> 2) & 0x4) | (extid & 0x3)) {
+	oobsize = ((chip->id.data[3] >> 2) & 0x3) |
+		  ((chip->id.data[3] >> 4) & 0x4);
+
+	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:
+			/*
+			 * We should never reach this case, but if that
+			 * happens, this probably means Hynix decided to use
+			 * a different extended ID format, and we should find
+			 * a way to support it.
+			 */
+			WARN(1, "Invalid OOB size");
+			break;
+		}
+	} else {
+		switch (oobsize) {
 		case 0:
 			mtd->oobsize = 128;
 			break;
@@ -49,23 +84,178 @@ static void hynix_nand_decode_id(struct nand_chip *chip)
 		case 5:
 			mtd->oobsize = 16;
 			break;
-		default:
+		case 6:
 			mtd->oobsize = 640;
 			break;
+		default:
+			/*
+			 * We should never reach this case, but if that
+			 * happens, this probably means Hynix decided to use
+			 * a different extended ID format, and we should find
+			 * a way to support it.
+			 */
+			WARN(1, "Invalid OOB size");
+			break;
 		}
+	}
+}
+
+static void 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;
 
-		/* Extract 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;
+		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:
+			/*
+			 * We should never reach this case, but if that
+			 * happens, this probably means Hynix decided to use
+			 * a different extended ID format, and we should find
+			 * a way to support it.
+			 */
+			WARN(1, "Invalid ECC requirements");
+		}
+	} 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 {
+				/*
+				 * We should never reach this case, but if that
+				 * happens, this probably means Hynix decided
+				 * to use a different extended ID format, and
+				 * we should find a way to support it.
+				 */
+				WARN(1, "Invalid ECC requirements");
+			}
+		} 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));
+			}
+		}
+	}
+}
+
+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;
+
+		/* < 32nm */
+		if (nand_tech > 2)
+			chip->options |= NAND_NEED_SCRAMBLING;
+	}
+}
+
+static void hynix_nand_decode_id(struct nand_chip *chip)
+{
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	bool valid_jedecid;
+	u8 tmp;
+
+	/*
+	 * 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)) {
 		nand_decode_ext_id(chip);
+		return;
 	}
+
+	/* Extract pagesize */
+	mtd->writesize = 2048 << (chip->id.data[3] & 0x03);
+
+	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);
+
+	hynix_nand_extract_oobsize(chip, valid_jedecid);
+	hynix_nand_extract_ecc_requirements(chip, valid_jedecid);
+	hynix_nand_extract_scrambling_requirements(chip, valid_jedecid);
 }
 
 static int hynix_nand_init(struct nand_chip *chip)
-- 
2.7.4