Driver is redesigned using parameter page to support all the Micron
SPI NAND flashes.
Parameter page of Micron flashes is similar to ONFI parameter table and
functionality is same, so copied some of the common functions like crc16
and bit_wise_majority from nand_onfi.c.
This driver is tested using MT29F2G01ABXGD, MT29F4G01ABXFD, MT29F8G01ADXFD,
MT29F1G01ABXFD.
Signed-off-by: Shivamurthy Shastri <[email protected]>
Reviewed-by: Bean Huo <[email protected]>
---
drivers/mtd/nand/spi/micron.c | 172 +++++++++++++++++++++++++++-------
drivers/mtd/nand/spi/micron.h | 83 ++++++++++++++++
2 files changed, 221 insertions(+), 34 deletions(-)
create mode 100644 drivers/mtd/nand/spi/micron.h
diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
index 9c4381d6847b..c9c53fd0aa01 100644
--- a/drivers/mtd/nand/spi/micron.c
+++ b/drivers/mtd/nand/spi/micron.c
@@ -10,13 +10,7 @@
#include <linux/kernel.h>
#include <linux/mtd/spinand.h>
-#define SPINAND_MFR_MICRON 0x2c
-
-#define MICRON_STATUS_ECC_MASK GENMASK(7, 4)
-#define MICRON_STATUS_ECC_NO_BITFLIPS (0 << 4)
-#define MICRON_STATUS_ECC_1TO3_BITFLIPS (1 << 4)
-#define MICRON_STATUS_ECC_4TO6_BITFLIPS (3 << 4)
-#define MICRON_STATUS_ECC_7TO8_BITFLIPS (5 << 4)
+#include "micron.h"
static SPINAND_OP_VARIANTS(read_cache_variants,
SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
@@ -34,38 +28,38 @@ static SPINAND_OP_VARIANTS(update_cache_variants,
SPINAND_PROG_LOAD_X4(false, 0, NULL, 0),
SPINAND_PROG_LOAD(false, 0, NULL, 0));
-static int mt29f2g01abagd_ooblayout_ecc(struct mtd_info *mtd, int section,
- struct mtd_oob_region *region)
+static int ooblayout_ecc(struct mtd_info *mtd, int section,
+ struct mtd_oob_region *region)
{
if (section)
return -ERANGE;
- region->offset = 64;
- region->length = 64;
+ region->offset = mtd->oobsize / 2;
+ region->length = mtd->oobsize / 2;
return 0;
}
-static int mt29f2g01abagd_ooblayout_free(struct mtd_info *mtd, int section,
- struct mtd_oob_region *region)
+static int ooblayout_free(struct mtd_info *mtd, int section,
+ struct mtd_oob_region *region)
{
if (section)
return -ERANGE;
/* Reserve 2 bytes for the BBM. */
region->offset = 2;
- region->length = 62;
+ region->length = (mtd->oobsize / 2) - 2;
return 0;
}
-static const struct mtd_ooblayout_ops mt29f2g01abagd_ooblayout = {
- .ecc = mt29f2g01abagd_ooblayout_ecc,
- .free = mt29f2g01abagd_ooblayout_free,
+static const struct mtd_ooblayout_ops ooblayout = {
+ .ecc = ooblayout_ecc,
+ .free = ooblayout_free,
};
-static int mt29f2g01abagd_ecc_get_status(struct spinand_device *spinand,
- u8 status)
+static int ecc_get_status(struct spinand_device *spinand,
+ u8 status)
{
switch (status & MICRON_STATUS_ECC_MASK) {
case STATUS_ECC_NO_BITFLIPS:
@@ -90,22 +84,53 @@ static int mt29f2g01abagd_ecc_get_status(struct spinand_device *spinand,
return -EINVAL;
}
-static const struct spinand_info micron_spinand_table[] = {
- SPINAND_INFO("MT29F2G01ABAGD", 0x24,
- NAND_MEMORG(1, 2048, 128, 64, 2048, 2, 1, 1),
- NAND_ECCREQ(8, 512),
- SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
- &write_cache_variants,
- &update_cache_variants),
- 0,
- SPINAND_ECCINFO(&mt29f2g01abagd_ooblayout,
- mt29f2g01abagd_ecc_get_status)),
-};
+static u16 spinand_crc16(u16 crc, u8 const *p, size_t len)
+{
+ int i;
+
+ while (len--) {
+ crc ^= *p++ << 8;
+ for (i = 0; i < 8; i++)
+ crc = (crc << 1) ^ ((crc & 0x8000) ? 0x8005 : 0);
+ }
+
+ return crc;
+}
+
+static void bit_wise_majority(const void **srcbufs,
+ unsigned int nsrcbufs,
+ void *dstbuf,
+ unsigned int bufsize)
+{
+ int i, j, k;
+
+ for (i = 0; i < bufsize; i++) {
+ u8 val = 0;
+
+ for (j = 0; j < 8; j++) {
+ unsigned int cnt = 0;
+
+ for (k = 0; k < nsrcbufs; k++) {
+ const u8 *srcbuf = srcbufs[k];
+
+ if (srcbuf[i] & BIT(j))
+ cnt++;
+ }
+
+ if (cnt > nsrcbufs / 2)
+ val |= BIT(j);
+ }
+
+ ((u8 *)dstbuf)[i] = val;
+ }
+}
static int micron_spinand_detect(struct spinand_device *spinand)
{
+ struct spinand_info deviceinfo;
+ struct micron_spinand_params *params;
u8 *id = spinand->id.data;
- int ret;
+ int ret, i;
/*
* Micron SPI NAND read ID need a dummy byte,
@@ -114,16 +139,95 @@ static int micron_spinand_detect(struct spinand_device *spinand)
if (id[1] != SPINAND_MFR_MICRON)
return 0;
- ret = spinand_match_and_init(spinand, micron_spinand_table,
- ARRAY_SIZE(micron_spinand_table), id[2]);
+ params = kzalloc(sizeof(*params) * 3, GFP_KERNEL);
+ if (!params)
+ return -ENOMEM;
+
+ ret = spinand_parameter_page_read(spinand, PARAMETER_PAGE, params,
+ sizeof(*params) * 3);
if (ret)
- return ret;
+ goto free_params;
+
+ for (i = 0; i < 3; i++) {
+ if (spinand_crc16(0x4F4E, (u8 *)¶ms[i], 254) ==
+ le16_to_cpu(params->crc)) {
+ if (i)
+ memcpy(params, ¶ms[i], sizeof(*params));
+ break;
+ }
+ }
+
+ if (i == 3) {
+ const void *srcbufs[3] = {params, params + 1, params + 2};
+
+ pr_warn("No valid parameter page, trying bit-wise majority to recover it\n");
+ bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), params,
+ sizeof(*params));
+
+ if (spinand_crc16(0x4F4E, (u8 *)params, 254) !=
+ le16_to_cpu(params->crc)) {
+ pr_err("Parameter page recovery failed, aborting\n");
+ goto free_params;
+ }
+ }
+
+ params->model[sizeof(params->model) - 1] = 0;
+ strim(params->model);
+
+ deviceinfo.model = kstrdup(params->model, GFP_KERNEL);
+ if (!deviceinfo.model) {
+ ret = -ENOMEM;
+ goto free_params;
+ }
+
+ deviceinfo.devid = id[2];
+ deviceinfo.flags = 0;
+ deviceinfo.memorg.bits_per_cell = params->bits_per_cell;
+ deviceinfo.memorg.pagesize = params->byte_per_page;
+ deviceinfo.memorg.oobsize = params->spare_bytes_per_page;
+ deviceinfo.memorg.pages_per_eraseblock = params->pages_per_block;
+ deviceinfo.memorg.eraseblocks_per_lun =
+ params->blocks_per_lun * params->lun_count;
+ deviceinfo.memorg.planes_per_lun = params->lun_count;
+ deviceinfo.memorg.luns_per_target = 1;
+ deviceinfo.memorg.ntargets = 1;
+ deviceinfo.eccreq.strength = params->ecc_max_correct_ability;
+ deviceinfo.eccreq.step_size = 512;
+ deviceinfo.eccinfo.get_status = ecc_get_status;
+ deviceinfo.eccinfo.ooblayout = &ooblayout;
+ deviceinfo.op_variants.read_cache = &read_cache_variants;
+ deviceinfo.op_variants.write_cache = &write_cache_variants;
+ deviceinfo.op_variants.update_cache = &update_cache_variants;
+
+ ret = spinand_match_and_init(spinand, &deviceinfo,
+ 1, id[2]);
+ if (ret)
+ goto free_model;
+
+ kfree(params);
return 1;
+
+free_model:
+ kfree(deviceinfo.model);
+free_params:
+ kfree(params);
+
+ return ret;
+}
+
+static int micron_spinand_init(struct spinand_device *spinand)
+{
+ /*
+ * Some of the Micron flashes enable this BIT by default,
+ * and there is a chance of read failure due to this.
+ */
+ return spinand_upd_cfg(spinand, CFG_QUAD_ENABLE, 0);
}
static const struct spinand_manufacturer_ops micron_spinand_manuf_ops = {
.detect = micron_spinand_detect,
+ .init = micron_spinand_init,
};
const struct spinand_manufacturer micron_spinand_manufacturer = {
diff --git a/drivers/mtd/nand/spi/micron.h b/drivers/mtd/nand/spi/micron.h
new file mode 100644
index 000000000000..c2cf3bee6f7e
--- /dev/null
+++ b/drivers/mtd/nand/spi/micron.h
@@ -0,0 +1,83 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2019 Micron Technology, Inc.
+ *
+ * Authors:
+ * Shivamurthy Shastri <[email protected]>
+ */
+
+#ifndef __MICRON_H
+#define __MICRON_H
+
+#define SPINAND_MFR_MICRON 0x2c
+
+#define MICRON_STATUS_ECC_MASK GENMASK(7, 4)
+#define MICRON_STATUS_ECC_NO_BITFLIPS (0 << 4)
+#define MICRON_STATUS_ECC_1TO3_BITFLIPS BIT(4)
+#define MICRON_STATUS_ECC_4TO6_BITFLIPS (3 << 4)
+#define MICRON_STATUS_ECC_7TO8_BITFLIPS (5 << 4)
+
+#define UNIQUE_ID_PAGE 0x00
+#define PARAMETER_PAGE 0x01
+
+/*
+ * Micron SPI NAND has parameter table similar to ONFI
+ */
+struct micron_spinand_params {
+ /* rev info and features block */
+ u8 sig[4];
+ __le16 revision;
+ __le16 features;
+ __le16 opt_cmd;
+ u8 reserved0[22];
+
+ /* manufacturer information block */
+ char manufacturer[12];
+ char model[20];
+ u8 manufact_id;
+ __le16 date_code;
+ u8 reserved1[13];
+
+ /* memory organization block */
+ __le32 byte_per_page;
+ __le16 spare_bytes_per_page;
+ __le32 data_bytes_per_ppage;
+ __le16 spare_bytes_per_ppage;
+ __le32 pages_per_block;
+ __le32 blocks_per_lun;
+ u8 lun_count;
+ u8 addr_cycles;
+ u8 bits_per_cell;
+ __le16 bb_per_lun;
+ __le16 block_endurance;
+ u8 guaranteed_good_blocks;
+ __le16 guaranteed_block_endurance;
+ u8 programs_per_page;
+ u8 ppage_attr;
+ u8 ecc_bits;
+ u8 interleaved_bits;
+ u8 interleaved_ops;
+ u8 reserved2[13];
+
+ /* electrical parameter block */
+ u8 io_pin_capacitance_max;
+ __le16 async_timing_mode;
+ __le16 program_cache_timing_mode;
+ __le16 t_prog;
+ __le16 t_bers;
+ __le16 t_r;
+ __le16 t_ccs;
+ u8 reserved3[23];
+
+ /* vendor */
+ __le16 vendor_revision;
+ u8 vendor_specific[14];
+ u8 reserved4[68];
+ u8 ecc_max_correct_ability;
+ u8 die_select_feature;
+ u8 reserved5[4];
+
+ __le16 crc;
+} __packed;
+
+#endif /* __MICRON_H */
--
2.17.1
Hi,
Den mån 4 feb. 2019 kl 12:18 skrev Shivamurthy Shastri (sshivamurthy)
<[email protected]>:
>
> Driver is redesigned using parameter page to support all the Micron
> SPI NAND flashes.
>
> Parameter page of Micron flashes is similar to ONFI parameter table and
> functionality is same, so copied some of the common functions like crc16
> and bit_wise_majority from nand_onfi.c.
>
> This driver is tested using MT29F2G01ABXGD, MT29F4G01ABXFD, MT29F8G01ADXFD,
> MT29F1G01ABXFD.
>
> -static const struct spinand_info micron_spinand_table[] = {
> - SPINAND_INFO("MT29F2G01ABAGD", 0x24,
> - NAND_MEMORG(1, 2048, 128, 64, 2048, 2, 1, 1),
> + deviceinfo.memorg.eraseblocks_per_lun =
> + params->blocks_per_lun * params->lun_count;
> + deviceinfo.memorg.planes_per_lun = params->lun_count;
> + deviceinfo.memorg.luns_per_target = 1;
> + deviceinfo.memorg.ntargets = 1;
> + __le32 blocks_per_lun;
> + u8 lun_count;
> + u8 addr_cycles;
> + u8 bits_per_cell;
> + __le16 bb_per_lun;
I have a question about the lun_count. As it is now, the
planes_per_lun parameter is initialized to 2 in NAND_MEMORG. In your
patch, it is instead initialized from the "lun_count" property from
the parameter table. But I looked at a datasheet I found by a simple
Google search (https://www.google.se/search?q=micron+nand+spi+datasheet),
the first hit is to the 1 Gb flash MT29F1G01AAADD. That device clearly
has two planes per lun (you need the "plane select" bit in the
requests), but still, according to the parameter page data structure,
byte 100, Number of logical units is set to 01h. Also, the "blocks per
lun" count, which is called "blocks per unit" is 1024, which should be
512 if this parameter really meant "blocks per plane" and the
calculation in the patch was correct.
As a reference, the 2 Gb version of the Macronix flash
(http://www.macronix.com/Lists/Datasheet/Attachments/6866/MX35LF2GE4AB,%203V,%202Gb,%20v1.5.pdf),
also has two planes per lun. It also sets byte 100, Number of logical
units to 01h.
So what I'm wondering is of course if this parameter is the correct
one to use for planes_per_lun. I tried to locate the correct "planes
per lun" parameter in the table, but didn't find anyone. Maybe it's
the unfortunate fact that "planes per lun" isn't exposed in the
parameter table?
/Emil
Hi Shivamurthy,
On Mon, 4 Feb 2019 11:17:51 +0000
"Shivamurthy Shastri (sshivamurthy)" <[email protected]> wrote:
> Driver is redesigned using parameter page to support all the Micron
> SPI NAND flashes.
Do all Micron SPI NANDs really expose a valid ONFI param page? If
that's not the case, then relying on ONFi parsing only sounds like a
bad idea.
>
> Parameter page of Micron flashes is similar to ONFI parameter table and
> functionality is same, so copied some of the common functions like crc16
> and bit_wise_majority from nand_onfi.c.
Most of the code is generic and does not depend on the spinand layer,
plus, we already have ONFI param page parsing code in
drivers/mtd/nand/raw/ which you're intentionally duplicating in a
version that will not be re-usable by the raw NAND layer even after
converting it to use the generic NAND layer.
Please move ONFi parsing code to drivers/mtd/nand/onfi.c and make it
generic.
>
> This driver is tested using MT29F2G01ABXGD, MT29F4G01ABXFD, MT29F8G01ADXFD,
> MT29F1G01ABXFD.
>
> Signed-off-by: Shivamurthy Shastri <[email protected]>
> Reviewed-by: Bean Huo <[email protected]>
I wish this code review had happened publicly.
> ---
> drivers/mtd/nand/spi/micron.c | 172 +++++++++++++++++++++++++++-------
> drivers/mtd/nand/spi/micron.h | 83 ++++++++++++++++
> 2 files changed, 221 insertions(+), 34 deletions(-)
> create mode 100644 drivers/mtd/nand/spi/micron.h
>
> diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
> index 9c4381d6847b..c9c53fd0aa01 100644
> --- a/drivers/mtd/nand/spi/micron.c
> +++ b/drivers/mtd/nand/spi/micron.c
> @@ -10,13 +10,7 @@
> #include <linux/kernel.h>
> #include <linux/mtd/spinand.h>
>
> -#define SPINAND_MFR_MICRON 0x2c
> -
> -#define MICRON_STATUS_ECC_MASK GENMASK(7, 4)
> -#define MICRON_STATUS_ECC_NO_BITFLIPS (0 << 4)
> -#define MICRON_STATUS_ECC_1TO3_BITFLIPS (1 << 4)
> -#define MICRON_STATUS_ECC_4TO6_BITFLIPS (3 << 4)
> -#define MICRON_STATUS_ECC_7TO8_BITFLIPS (5 << 4)
> +#include "micron.h"
>
> static SPINAND_OP_VARIANTS(read_cache_variants,
> SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
> @@ -34,38 +28,38 @@ static SPINAND_OP_VARIANTS(update_cache_variants,
> SPINAND_PROG_LOAD_X4(false, 0, NULL, 0),
> SPINAND_PROG_LOAD(false, 0, NULL, 0));
>
> -static int mt29f2g01abagd_ooblayout_ecc(struct mtd_info *mtd, int section,
> - struct mtd_oob_region *region)
> +static int ooblayout_ecc(struct mtd_info *mtd, int section,
> + struct mtd_oob_region *region)
> {
> if (section)
> return -ERANGE;
>
> - region->offset = 64;
> - region->length = 64;
> + region->offset = mtd->oobsize / 2;
> + region->length = mtd->oobsize / 2;
>
> return 0;
> }
>
> -static int mt29f2g01abagd_ooblayout_free(struct mtd_info *mtd, int section,
> - struct mtd_oob_region *region)
> +static int ooblayout_free(struct mtd_info *mtd, int section,
> + struct mtd_oob_region *region)
> {
> if (section)
> return -ERANGE;
>
> /* Reserve 2 bytes for the BBM. */
> region->offset = 2;
> - region->length = 62;
> + region->length = (mtd->oobsize / 2) - 2;
>
> return 0;
> }
>
> -static const struct mtd_ooblayout_ops mt29f2g01abagd_ooblayout = {
> - .ecc = mt29f2g01abagd_ooblayout_ecc,
> - .free = mt29f2g01abagd_ooblayout_free,
> +static const struct mtd_ooblayout_ops ooblayout = {
> + .ecc = ooblayout_ecc,
> + .free = ooblayout_free,
> };
>
> -static int mt29f2g01abagd_ecc_get_status(struct spinand_device *spinand,
> - u8 status)
> +static int ecc_get_status(struct spinand_device *spinand,
> + u8 status)
> {
> switch (status & MICRON_STATUS_ECC_MASK) {
> case STATUS_ECC_NO_BITFLIPS:
> @@ -90,22 +84,53 @@ static int mt29f2g01abagd_ecc_get_status(struct spinand_device *spinand,
> return -EINVAL;
> }
>
> -static const struct spinand_info micron_spinand_table[] = {
> - SPINAND_INFO("MT29F2G01ABAGD", 0x24,
> - NAND_MEMORG(1, 2048, 128, 64, 2048, 2, 1, 1),
> - NAND_ECCREQ(8, 512),
> - SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> - &write_cache_variants,
> - &update_cache_variants),
> - 0,
> - SPINAND_ECCINFO(&mt29f2g01abagd_ooblayout,
> - mt29f2g01abagd_ecc_get_status)),
> -};
> +static u16 spinand_crc16(u16 crc, u8 const *p, size_t len)
> +{
> + int i;
> +
> + while (len--) {
> + crc ^= *p++ << 8;
> + for (i = 0; i < 8; i++)
> + crc = (crc << 1) ^ ((crc & 0x8000) ? 0x8005 : 0);
> + }
> +
> + return crc;
> +}
> +
> +static void bit_wise_majority(const void **srcbufs,
> + unsigned int nsrcbufs,
> + void *dstbuf,
> + unsigned int bufsize)
> +{
> + int i, j, k;
> +
> + for (i = 0; i < bufsize; i++) {
> + u8 val = 0;
> +
> + for (j = 0; j < 8; j++) {
> + unsigned int cnt = 0;
> +
> + for (k = 0; k < nsrcbufs; k++) {
> + const u8 *srcbuf = srcbufs[k];
> +
> + if (srcbuf[i] & BIT(j))
> + cnt++;
> + }
> +
> + if (cnt > nsrcbufs / 2)
> + val |= BIT(j);
> + }
> +
> + ((u8 *)dstbuf)[i] = val;
> + }
> +}
>
> static int micron_spinand_detect(struct spinand_device *spinand)
> {
> + struct spinand_info deviceinfo;
> + struct micron_spinand_params *params;
> u8 *id = spinand->id.data;
> - int ret;
> + int ret, i;
>
> /*
> * Micron SPI NAND read ID need a dummy byte,
> @@ -114,16 +139,95 @@ static int micron_spinand_detect(struct spinand_device *spinand)
> if (id[1] != SPINAND_MFR_MICRON)
> return 0;
>
> - ret = spinand_match_and_init(spinand, micron_spinand_table,
> - ARRAY_SIZE(micron_spinand_table), id[2]);
> + params = kzalloc(sizeof(*params) * 3, GFP_KERNEL);
> + if (!params)
> + return -ENOMEM;
> +
> + ret = spinand_parameter_page_read(spinand, PARAMETER_PAGE, params,
> + sizeof(*params) * 3);
> if (ret)
> - return ret;
> + goto free_params;
> +
> + for (i = 0; i < 3; i++) {
> + if (spinand_crc16(0x4F4E, (u8 *)¶ms[i], 254) ==
> + le16_to_cpu(params->crc)) {
> + if (i)
> + memcpy(params, ¶ms[i], sizeof(*params));
> + break;
> + }
> + }
> +
> + if (i == 3) {
> + const void *srcbufs[3] = {params, params + 1, params + 2};
> +
> + pr_warn("No valid parameter page, trying bit-wise majority to recover it\n");
> + bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), params,
> + sizeof(*params));
> +
> + if (spinand_crc16(0x4F4E, (u8 *)params, 254) !=
> + le16_to_cpu(params->crc)) {
> + pr_err("Parameter page recovery failed, aborting\n");
> + goto free_params;
> + }
> + }
> +
> + params->model[sizeof(params->model) - 1] = 0;
> + strim(params->model);
> +
> + deviceinfo.model = kstrdup(params->model, GFP_KERNEL);
> + if (!deviceinfo.model) {
> + ret = -ENOMEM;
> + goto free_params;
> + }
> +
> + deviceinfo.devid = id[2];
> + deviceinfo.flags = 0;
> + deviceinfo.memorg.bits_per_cell = params->bits_per_cell;
> + deviceinfo.memorg.pagesize = params->byte_per_page;
> + deviceinfo.memorg.oobsize = params->spare_bytes_per_page;
> + deviceinfo.memorg.pages_per_eraseblock = params->pages_per_block;
> + deviceinfo.memorg.eraseblocks_per_lun =
> + params->blocks_per_lun * params->lun_count;
> + deviceinfo.memorg.planes_per_lun = params->lun_count;
As pointed by Emil, this is wrong, params->lun_count should be used to
fill luns_per_target. ->planes_per_lun should be extracted from
->interleaved_bits.
> + deviceinfo.memorg.luns_per_target = 1;
> + deviceinfo.memorg.ntargets = 1;
> + deviceinfo.eccreq.strength = params->ecc_max_correct_ability;
> + deviceinfo.eccreq.step_size = 512;
> + deviceinfo.eccinfo.get_status = ecc_get_status;
> + deviceinfo.eccinfo.ooblayout = &ooblayout;
Are all devices really using the same get_status method and the same
layout. Sounds risky to me to assume this is the case.
> + deviceinfo.op_variants.read_cache = &read_cache_variants;
> + deviceinfo.op_variants.write_cache = &write_cache_variants;
> + deviceinfo.op_variants.update_cache = &update_cache_variants;
> +
> + ret = spinand_match_and_init(spinand, &deviceinfo,
> + 1, id[2]);
Please don't abuse the spinand_match_and_init() function. Fill the
nand_device object directly instead of creating a temporary spinand_info
instance with the expected id.
> + if (ret)
> + goto free_model;
> +
> + kfree(params);
>
> return 1;
> +
> +free_model:
> + kfree(deviceinfo.model);
> +free_params:
> + kfree(params);
> +
> + return ret;
> +}
> +
> +static int micron_spinand_init(struct spinand_device *spinand)
> +{
> + /*
> + * Some of the Micron flashes enable this BIT by default,
> + * and there is a chance of read failure due to this.
> + */
> + return spinand_upd_cfg(spinand, CFG_QUAD_ENABLE, 0);
> }
>
> static const struct spinand_manufacturer_ops micron_spinand_manuf_ops = {
> .detect = micron_spinand_detect,
> + .init = micron_spinand_init,
> };
>
> const struct spinand_manufacturer micron_spinand_manufacturer = {
> diff --git a/drivers/mtd/nand/spi/micron.h b/drivers/mtd/nand/spi/micron.h
> new file mode 100644
> index 000000000000..c2cf3bee6f7e
> --- /dev/null
> +++ b/drivers/mtd/nand/spi/micron.h
> @@ -0,0 +1,83 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2019 Micron Technology, Inc.
> + *
> + * Authors:
> + * Shivamurthy Shastri <[email protected]>
> + */
> +
> +#ifndef __MICRON_H
> +#define __MICRON_H
> +
> +#define SPINAND_MFR_MICRON 0x2c
> +
> +#define MICRON_STATUS_ECC_MASK GENMASK(7, 4)
> +#define MICRON_STATUS_ECC_NO_BITFLIPS (0 << 4)
> +#define MICRON_STATUS_ECC_1TO3_BITFLIPS BIT(4)
> +#define MICRON_STATUS_ECC_4TO6_BITFLIPS (3 << 4)
> +#define MICRON_STATUS_ECC_7TO8_BITFLIPS (5 << 4)
> +
> +#define UNIQUE_ID_PAGE 0x00
> +#define PARAMETER_PAGE 0x01
> +
> +/*
> + * Micron SPI NAND has parameter table similar to ONFI
> + */
> +struct micron_spinand_params {
> + /* rev info and features block */
> + u8 sig[4];
> + __le16 revision;
> + __le16 features;
> + __le16 opt_cmd;
> + u8 reserved0[22];
> +
> + /* manufacturer information block */
> + char manufacturer[12];
> + char model[20];
> + u8 manufact_id;
> + __le16 date_code;
> + u8 reserved1[13];
> +
> + /* memory organization block */
> + __le32 byte_per_page;
> + __le16 spare_bytes_per_page;
> + __le32 data_bytes_per_ppage;
> + __le16 spare_bytes_per_ppage;
> + __le32 pages_per_block;
> + __le32 blocks_per_lun;
> + u8 lun_count;
> + u8 addr_cycles;
> + u8 bits_per_cell;
> + __le16 bb_per_lun;
> + __le16 block_endurance;
> + u8 guaranteed_good_blocks;
> + __le16 guaranteed_block_endurance;
> + u8 programs_per_page;
> + u8 ppage_attr;
> + u8 ecc_bits;
> + u8 interleaved_bits;
> + u8 interleaved_ops;
> + u8 reserved2[13];
> +
> + /* electrical parameter block */
> + u8 io_pin_capacitance_max;
> + __le16 async_timing_mode;
> + __le16 program_cache_timing_mode;
> + __le16 t_prog;
> + __le16 t_bers;
> + __le16 t_r;
> + __le16 t_ccs;
> + u8 reserved3[23];
> +
> + /* vendor */
> + __le16 vendor_revision;
> + u8 vendor_specific[14];
> + u8 reserved4[68];
> + u8 ecc_max_correct_ability;
> + u8 die_select_feature;
> + u8 reserved5[4];
> +
> + __le16 crc;
> +} __packed;
> +
Please use the nand_onfi_params definition (include/linux/mtd/onfi.h).
> +#endif /* __MICRON_H */
Regards,
Boris
Hi Emil,
> Hi,
>
> Den mån 4 feb. 2019 kl 12:18 skrev Shivamurthy Shastri (sshivamurthy)
> <[email protected]>:
> >
> > Driver is redesigned using parameter page to support all the Micron
> > SPI NAND flashes.
> >
> > Parameter page of Micron flashes is similar to ONFI parameter table
> > and functionality is same, so copied some of the common functions like
> > crc16 and bit_wise_majority from nand_onfi.c.
> >
> > This driver is tested using MT29F2G01ABXGD, MT29F4G01ABXFD,
> > MT29F8G01ADXFD, MT29F1G01ABXFD.
> >
>
> > -static const struct spinand_info micron_spinand_table[] = {
> > - SPINAND_INFO("MT29F2G01ABAGD", 0x24,
> > - NAND_MEMORG(1, 2048, 128, 64, 2048, 2, 1, 1),
>
> > + deviceinfo.memorg.eraseblocks_per_lun =
> > + params->blocks_per_lun * params->lun_count;
> > + deviceinfo.memorg.planes_per_lun = params->lun_count;
> > + deviceinfo.memorg.luns_per_target = 1;
> > + deviceinfo.memorg.ntargets = 1;
>
> > + __le32 blocks_per_lun;
> > + u8 lun_count;
> > + u8 addr_cycles;
> > + u8 bits_per_cell;
> > + __le16 bb_per_lun;
>
> I have a question about the lun_count. As it is now, the planes_per_lun
> parameter is initialized to 2 in NAND_MEMORG. In your patch, it is instead
> initialized from the "lun_count" property from the parameter table. But I
> looked at a datasheet I found by a simple Google search
> (https://www.google.se/search?q=micron+nand+spi+datasheet),
> the first hit is to the 1 Gb flash MT29F1G01AAADD. That device clearly has
> two planes per lun (you need the "plane select" bit in the requests), but still,
> according to the parameter page data structure, byte 100, Number of logical
> units is set to 01h. Also, the "blocks per lun" count, which is called "blocks per
> unit" is 1024, which should be
> 512 if this parameter really meant "blocks per plane" and the calculation in
> the patch was correct.
You are right, I somehow didn't send the correct patch for this.
This is my first time sending patch to mainline, lost in nervousness.
In micron flashes we can identify number of planes in LUN using
parameter page Byte 166 -> vendor_specific[0].
Byte 166 will be "1", if LUN has more than one plane.
I will resend the correct patch.
However, the device you mentioned is obsolete, very old and not in production.
This code will not support that.
Please refer new datasheets mentioned in the commit messages.
>
> As a reference, the 2 Gb version of the Macronix flash
> (http://www.macronix.com/Lists/Datasheet/Attachments/6866/MX35LF2GE
> 4AB,%203V,%202Gb,%20v1.5.pdf),
> also has two planes per lun. It also sets byte 100, Number of logical units to
> 01h.
>
> So what I'm wondering is of course if this parameter is the correct one to use
> for planes_per_lun. I tried to locate the correct "planes per lun" parameter in
> the table, but didn't find anyone. Maybe it's the unfortunate fact that
> "planes per lun" isn't exposed in the parameter table?
>
> /Emil
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
Hi Shivamurthy,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on mtd/nand/next]
[also build test WARNING on v5.0-rc4 next-20190204]
[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/Shivamurthy-Shastri-sshivamurthy/Support-parameter-page-and-Redesign-Micron-SPI-NAND/20190205-012740
base: git://git.infradead.org/linux-mtd.git nand/next
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
All warnings (new ones prefixed by >>):
>> drivers/mtd/nand/spi/micron.c:186:36: sparse: warning: incorrect type in assignment (different base types)
drivers/mtd/nand/spi/micron.c:186:36: sparse: expected unsigned int [assigned] pagesize
drivers/mtd/nand/spi/micron.c:186:36: sparse: got restricted __le32 [usertype] byte_per_page
drivers/mtd/nand/spi/micron.c:187:35: sparse: warning: incorrect type in assignment (different base types)
drivers/mtd/nand/spi/micron.c:187:35: sparse: expected unsigned int [assigned] oobsize
drivers/mtd/nand/spi/micron.c:187:35: sparse: got restricted __le16 [usertype] spare_bytes_per_page
drivers/mtd/nand/spi/micron.c:188:48: sparse: warning: incorrect type in assignment (different base types)
drivers/mtd/nand/spi/micron.c:188:48: sparse: expected unsigned int [assigned] pages_per_eraseblock
drivers/mtd/nand/spi/micron.c:188:48: sparse: got restricted __le32 [usertype] pages_per_block
>> drivers/mtd/nand/spi/micron.c:190:23: sparse: warning: restricted __le32 degrades to integer
sparse warnings: (new ones prefixed by >>)
drivers/mtd/nand/spi/micron.c:186:36: sparse: warning: incorrect type in assignment (different base types)
>> drivers/mtd/nand/spi/micron.c:186:36: sparse: expected unsigned int [assigned] pagesize
>> drivers/mtd/nand/spi/micron.c:186:36: sparse: got restricted __le32 [usertype] byte_per_page
drivers/mtd/nand/spi/micron.c:187:35: sparse: warning: incorrect type in assignment (different base types)
>> drivers/mtd/nand/spi/micron.c:187:35: sparse: expected unsigned int [assigned] oobsize
>> drivers/mtd/nand/spi/micron.c:187:35: sparse: got restricted __le16 [usertype] spare_bytes_per_page
drivers/mtd/nand/spi/micron.c:188:48: sparse: warning: incorrect type in assignment (different base types)
>> drivers/mtd/nand/spi/micron.c:188:48: sparse: expected unsigned int [assigned] pages_per_eraseblock
>> drivers/mtd/nand/spi/micron.c:188:48: sparse: got restricted __le32 [usertype] pages_per_block
drivers/mtd/nand/spi/micron.c:190:23: sparse: warning: restricted __le32 degrades to integer
vim +186 drivers/mtd/nand/spi/micron.c
127
128 static int micron_spinand_detect(struct spinand_device *spinand)
129 {
130 struct spinand_info deviceinfo;
131 struct micron_spinand_params *params;
132 u8 *id = spinand->id.data;
133 int ret, i;
134
135 /*
136 * Micron SPI NAND read ID need a dummy byte,
137 * so the first byte in raw_id is dummy.
138 */
139 if (id[1] != SPINAND_MFR_MICRON)
140 return 0;
141
142 params = kzalloc(sizeof(*params) * 3, GFP_KERNEL);
143 if (!params)
144 return -ENOMEM;
145
146 ret = spinand_parameter_page_read(spinand, PARAMETER_PAGE, params,
147 sizeof(*params) * 3);
148 if (ret)
149 goto free_params;
150
151 for (i = 0; i < 3; i++) {
152 if (spinand_crc16(0x4F4E, (u8 *)¶ms[i], 254) ==
153 le16_to_cpu(params->crc)) {
154 if (i)
155 memcpy(params, ¶ms[i], sizeof(*params));
156 break;
157 }
158 }
159
160 if (i == 3) {
161 const void *srcbufs[3] = {params, params + 1, params + 2};
162
163 pr_warn("No valid parameter page, trying bit-wise majority to recover it\n");
164 bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), params,
165 sizeof(*params));
166
167 if (spinand_crc16(0x4F4E, (u8 *)params, 254) !=
168 le16_to_cpu(params->crc)) {
169 pr_err("Parameter page recovery failed, aborting\n");
170 goto free_params;
171 }
172 }
173
174 params->model[sizeof(params->model) - 1] = 0;
175 strim(params->model);
176
177 deviceinfo.model = kstrdup(params->model, GFP_KERNEL);
178 if (!deviceinfo.model) {
179 ret = -ENOMEM;
180 goto free_params;
181 }
182
183 deviceinfo.devid = id[2];
184 deviceinfo.flags = 0;
185 deviceinfo.memorg.bits_per_cell = params->bits_per_cell;
> 186 deviceinfo.memorg.pagesize = params->byte_per_page;
> 187 deviceinfo.memorg.oobsize = params->spare_bytes_per_page;
> 188 deviceinfo.memorg.pages_per_eraseblock = params->pages_per_block;
189 deviceinfo.memorg.eraseblocks_per_lun =
> 190 params->blocks_per_lun * params->lun_count;
191 deviceinfo.memorg.planes_per_lun = params->lun_count;
192 deviceinfo.memorg.luns_per_target = 1;
193 deviceinfo.memorg.ntargets = 1;
194 deviceinfo.eccreq.strength = params->ecc_max_correct_ability;
195 deviceinfo.eccreq.step_size = 512;
196 deviceinfo.eccinfo.get_status = ecc_get_status;
197 deviceinfo.eccinfo.ooblayout = &ooblayout;
198 deviceinfo.op_variants.read_cache = &read_cache_variants;
199 deviceinfo.op_variants.write_cache = &write_cache_variants;
200 deviceinfo.op_variants.update_cache = &update_cache_variants;
201
202 ret = spinand_match_and_init(spinand, &deviceinfo,
203 1, id[2]);
204 if (ret)
205 goto free_model;
206
207 kfree(params);
208
209 return 1;
210
211 free_model:
212 kfree(deviceinfo.model);
213 free_params:
214 kfree(params);
215
216 return ret;
217 }
218
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Boris,
> -----Original Message-----
> From: Boris Brezillon <[email protected]>
> Sent: Monday, February 4, 2019 7:02 PM
> To: Shivamurthy Shastri (sshivamurthy) <[email protected]>
> Cc: Miquel Raynal <[email protected]>; linux-
> [email protected]; [email protected]; Chuanhong Guo
> <[email protected]>; Richard Weinberger <[email protected]>; Schrempf
> Frieder <[email protected]>; Marek Vasut
> <[email protected]>; Frieder Schrempf
> <[email protected]>; Brian Norris
> <[email protected]>; David Woodhouse
> <[email protected]>; Bean Huo (beanhuo) <[email protected]>
> Subject: [EXT] Re: [PATCH 2/2] mtd: spinand: micron: Support for all Micron
> SPI NAND flashes
>
> Hi Shivamurthy,
>
> On Mon, 4 Feb 2019 11:17:51 +0000
> "Shivamurthy Shastri (sshivamurthy)" <[email protected]> wrote:
>
> > Driver is redesigned using parameter page to support all the Micron
> > SPI NAND flashes.
>
> Do all Micron SPI NANDs really expose a valid ONFI param page? If
> that's not the case, then relying on ONFi parsing only sounds like a
> bad idea.
Micron SPI NAND datasheet does not confirm to be as ONFI standard.
However, they all expose parameter page, which I used for development.
>
> >
> > Parameter page of Micron flashes is similar to ONFI parameter table and
> > functionality is same, so copied some of the common functions like crc16
> > and bit_wise_majority from nand_onfi.c.
>
> Most of the code is generic and does not depend on the spinand layer,
> plus, we already have ONFI param page parsing code in
> drivers/mtd/nand/raw/ which you're intentionally duplicating in a
> version that will not be re-usable by the raw NAND layer even after
> converting it to use the generic NAND layer.
>
> Please move ONFi parsing code to drivers/mtd/nand/onfi.c and make it
> generic.
As I said before, it is not compliant to ONFI standard, I think it is better not
to make it generic.
>
> >
> > This driver is tested using MT29F2G01ABXGD, MT29F4G01ABXFD,
> MT29F8G01ADXFD,
> > MT29F1G01ABXFD.
> >
> > Signed-off-by: Shivamurthy Shastri <[email protected]>
> > Reviewed-by: Bean Huo <[email protected]>
>
> I wish this code review had happened publicly.
>
> > ---
> > drivers/mtd/nand/spi/micron.c | 172 +++++++++++++++++++++++++++--
> -----
> > drivers/mtd/nand/spi/micron.h | 83 ++++++++++++++++
> > 2 files changed, 221 insertions(+), 34 deletions(-)
> > create mode 100644 drivers/mtd/nand/spi/micron.h
> >
> > diff --git a/drivers/mtd/nand/spi/micron.c
> b/drivers/mtd/nand/spi/micron.c
> > index 9c4381d6847b..c9c53fd0aa01 100644
> > --- a/drivers/mtd/nand/spi/micron.c
> > +++ b/drivers/mtd/nand/spi/micron.c
> > @@ -10,13 +10,7 @@
> > #include <linux/kernel.h>
> > #include <linux/mtd/spinand.h>
> >
> > -#define SPINAND_MFR_MICRON 0x2c
> > -
> > -#define MICRON_STATUS_ECC_MASK GENMASK(7, 4)
> > -#define MICRON_STATUS_ECC_NO_BITFLIPS (0 << 4)
> > -#define MICRON_STATUS_ECC_1TO3_BITFLIPS (1 << 4)
> > -#define MICRON_STATUS_ECC_4TO6_BITFLIPS (3 << 4)
> > -#define MICRON_STATUS_ECC_7TO8_BITFLIPS (5 << 4)
> > +#include "micron.h"
> >
> > static SPINAND_OP_VARIANTS(read_cache_variants,
> > SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2,
> NULL, 0),
> > @@ -34,38 +28,38 @@ static
> SPINAND_OP_VARIANTS(update_cache_variants,
> > SPINAND_PROG_LOAD_X4(false, 0, NULL, 0),
> > SPINAND_PROG_LOAD(false, 0, NULL, 0));
> >
> > -static int mt29f2g01abagd_ooblayout_ecc(struct mtd_info *mtd, int
> section,
> > - struct mtd_oob_region *region)
> > +static int ooblayout_ecc(struct mtd_info *mtd, int section,
> > + struct mtd_oob_region *region)
> > {
> > if (section)
> > return -ERANGE;
> >
> > - region->offset = 64;
> > - region->length = 64;
> > + region->offset = mtd->oobsize / 2;
> > + region->length = mtd->oobsize / 2;
> >
> > return 0;
> > }
> >
> > -static int mt29f2g01abagd_ooblayout_free(struct mtd_info *mtd, int
> section,
> > - struct mtd_oob_region *region)
> > +static int ooblayout_free(struct mtd_info *mtd, int section,
> > + struct mtd_oob_region *region)
> > {
> > if (section)
> > return -ERANGE;
> >
> > /* Reserve 2 bytes for the BBM. */
> > region->offset = 2;
> > - region->length = 62;
> > + region->length = (mtd->oobsize / 2) - 2;
> >
> > return 0;
> > }
> >
> > -static const struct mtd_ooblayout_ops mt29f2g01abagd_ooblayout = {
> > - .ecc = mt29f2g01abagd_ooblayout_ecc,
> > - .free = mt29f2g01abagd_ooblayout_free,
> > +static const struct mtd_ooblayout_ops ooblayout = {
> > + .ecc = ooblayout_ecc,
> > + .free = ooblayout_free,
> > };
> >
> > -static int mt29f2g01abagd_ecc_get_status(struct spinand_device
> *spinand,
> > - u8 status)
> > +static int ecc_get_status(struct spinand_device *spinand,
> > + u8 status)
> > {
> > switch (status & MICRON_STATUS_ECC_MASK) {
> > case STATUS_ECC_NO_BITFLIPS:
> > @@ -90,22 +84,53 @@ static int mt29f2g01abagd_ecc_get_status(struct
> spinand_device *spinand,
> > return -EINVAL;
> > }
> >
> > -static const struct spinand_info micron_spinand_table[] = {
> > - SPINAND_INFO("MT29F2G01ABAGD", 0x24,
> > - NAND_MEMORG(1, 2048, 128, 64, 2048, 2, 1, 1),
> > - NAND_ECCREQ(8, 512),
> > - SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > - &write_cache_variants,
> > - &update_cache_variants),
> > - 0,
> > - SPINAND_ECCINFO(&mt29f2g01abagd_ooblayout,
> > - mt29f2g01abagd_ecc_get_status)),
> > -};
> > +static u16 spinand_crc16(u16 crc, u8 const *p, size_t len)
> > +{
> > + int i;
> > +
> > + while (len--) {
> > + crc ^= *p++ << 8;
> > + for (i = 0; i < 8; i++)
> > + crc = (crc << 1) ^ ((crc & 0x8000) ? 0x8005 : 0);
> > + }
> > +
> > + return crc;
> > +}
> > +
> > +static void bit_wise_majority(const void **srcbufs,
> > + unsigned int nsrcbufs,
> > + void *dstbuf,
> > + unsigned int bufsize)
> > +{
> > + int i, j, k;
> > +
> > + for (i = 0; i < bufsize; i++) {
> > + u8 val = 0;
> > +
> > + for (j = 0; j < 8; j++) {
> > + unsigned int cnt = 0;
> > +
> > + for (k = 0; k < nsrcbufs; k++) {
> > + const u8 *srcbuf = srcbufs[k];
> > +
> > + if (srcbuf[i] & BIT(j))
> > + cnt++;
> > + }
> > +
> > + if (cnt > nsrcbufs / 2)
> > + val |= BIT(j);
> > + }
> > +
> > + ((u8 *)dstbuf)[i] = val;
> > + }
> > +}
> >
> > static int micron_spinand_detect(struct spinand_device *spinand)
> > {
> > + struct spinand_info deviceinfo;
> > + struct micron_spinand_params *params;
> > u8 *id = spinand->id.data;
> > - int ret;
> > + int ret, i;
> >
> > /*
> > * Micron SPI NAND read ID need a dummy byte,
> > @@ -114,16 +139,95 @@ static int micron_spinand_detect(struct
> spinand_device *spinand)
> > if (id[1] != SPINAND_MFR_MICRON)
> > return 0;
> >
> > - ret = spinand_match_and_init(spinand, micron_spinand_table,
> > - ARRAY_SIZE(micron_spinand_table),
> id[2]);
> > + params = kzalloc(sizeof(*params) * 3, GFP_KERNEL);
> > + if (!params)
> > + return -ENOMEM;
> > +
> > + ret = spinand_parameter_page_read(spinand, PARAMETER_PAGE,
> params,
> > + sizeof(*params) * 3);
> > if (ret)
> > - return ret;
> > + goto free_params;
> > +
> > + for (i = 0; i < 3; i++) {
> > + if (spinand_crc16(0x4F4E, (u8 *)¶ms[i], 254) ==
> > + le16_to_cpu(params->crc)) {
> > + if (i)
> > + memcpy(params, ¶ms[i],
> sizeof(*params));
> > + break;
> > + }
> > + }
> > +
> > + if (i == 3) {
> > + const void *srcbufs[3] = {params, params + 1, params + 2};
> > +
> > + pr_warn("No valid parameter page, trying bit-wise majority
> to recover it\n");
> > + bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), params,
> > + sizeof(*params));
> > +
> > + if (spinand_crc16(0x4F4E, (u8 *)params, 254) !=
> > + le16_to_cpu(params->crc)) {
> > + pr_err("Parameter page recovery failed,
> aborting\n");
> > + goto free_params;
> > + }
> > + }
> > +
> > + params->model[sizeof(params->model) - 1] = 0;
> > + strim(params->model);
> > +
> > + deviceinfo.model = kstrdup(params->model, GFP_KERNEL);
> > + if (!deviceinfo.model) {
> > + ret = -ENOMEM;
> > + goto free_params;
> > + }
> > +
> > + deviceinfo.devid = id[2];
> > + deviceinfo.flags = 0;
> > + deviceinfo.memorg.bits_per_cell = params->bits_per_cell;
> > + deviceinfo.memorg.pagesize = params->byte_per_page;
> > + deviceinfo.memorg.oobsize = params->spare_bytes_per_page;
> > + deviceinfo.memorg.pages_per_eraseblock = params-
> >pages_per_block;
> > + deviceinfo.memorg.eraseblocks_per_lun =
> > + params->blocks_per_lun * params->lun_count;
> > + deviceinfo.memorg.planes_per_lun = params->lun_count;
>
> As pointed by Emil, this is wrong, params->lun_count should be used to
> fill luns_per_target. ->planes_per_lun should be extracted from
> ->interleaved_bits.
It is my bad, I will correct it.
However, Micron SPI NAND's use vendor specific area ->vendor_specific[0] .
>
> > + deviceinfo.memorg.luns_per_target = 1;
> > + deviceinfo.memorg.ntargets = 1;
> > + deviceinfo.eccreq.strength = params->ecc_max_correct_ability;
> > + deviceinfo.eccreq.step_size = 512;
> > + deviceinfo.eccinfo.get_status = ecc_get_status;
> > + deviceinfo.eccinfo.ooblayout = &ooblayout;
>
> Are all devices really using the same get_status method and the same
> layout. Sounds risky to me to assume this is the case.
>
> > + deviceinfo.op_variants.read_cache = &read_cache_variants;
> > + deviceinfo.op_variants.write_cache = &write_cache_variants;
> > + deviceinfo.op_variants.update_cache = &update_cache_variants;
> > +
> > + ret = spinand_match_and_init(spinand, &deviceinfo,
> > + 1, id[2]);
>
> Please don't abuse the spinand_match_and_init() function. Fill the
> nand_device object directly instead of creating a temporary spinand_info
> instance with the expected id.
>
> > + if (ret)
> > + goto free_model;
> > +
> > + kfree(params);
> >
> > return 1;
> > +
> > +free_model:
> > + kfree(deviceinfo.model);
> > +free_params:
> > + kfree(params);
> > +
> > + return ret;
> > +}
> > +
> > +static int micron_spinand_init(struct spinand_device *spinand)
> > +{
> > + /*
> > + * Some of the Micron flashes enable this BIT by default,
> > + * and there is a chance of read failure due to this.
> > + */
> > + return spinand_upd_cfg(spinand, CFG_QUAD_ENABLE, 0);
> > }
> >
> > static const struct spinand_manufacturer_ops
> micron_spinand_manuf_ops = {
> > .detect = micron_spinand_detect,
> > + .init = micron_spinand_init,
> > };
> >
> > const struct spinand_manufacturer micron_spinand_manufacturer = {
> > diff --git a/drivers/mtd/nand/spi/micron.h
> b/drivers/mtd/nand/spi/micron.h
> > new file mode 100644
> > index 000000000000..c2cf3bee6f7e
> > --- /dev/null
> > +++ b/drivers/mtd/nand/spi/micron.h
> > @@ -0,0 +1,83 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2019 Micron Technology, Inc.
> > + *
> > + * Authors:
> > + * Shivamurthy Shastri <[email protected]>
> > + */
> > +
> > +#ifndef __MICRON_H
> > +#define __MICRON_H
> > +
> > +#define SPINAND_MFR_MICRON 0x2c
> > +
> > +#define MICRON_STATUS_ECC_MASK GENMASK(7, 4)
> > +#define MICRON_STATUS_ECC_NO_BITFLIPS (0 << 4)
> > +#define MICRON_STATUS_ECC_1TO3_BITFLIPS BIT(4)
> > +#define MICRON_STATUS_ECC_4TO6_BITFLIPS (3 << 4)
> > +#define MICRON_STATUS_ECC_7TO8_BITFLIPS (5 << 4)
> > +
> > +#define UNIQUE_ID_PAGE 0x00
> > +#define PARAMETER_PAGE 0x01
> > +
> > +/*
> > + * Micron SPI NAND has parameter table similar to ONFI
> > + */
> > +struct micron_spinand_params {
> > + /* rev info and features block */
> > + u8 sig[4];
> > + __le16 revision;
> > + __le16 features;
> > + __le16 opt_cmd;
> > + u8 reserved0[22];
> > +
> > + /* manufacturer information block */
> > + char manufacturer[12];
> > + char model[20];
> > + u8 manufact_id;
> > + __le16 date_code;
> > + u8 reserved1[13];
> > +
> > + /* memory organization block */
> > + __le32 byte_per_page;
> > + __le16 spare_bytes_per_page;
> > + __le32 data_bytes_per_ppage;
> > + __le16 spare_bytes_per_ppage;
> > + __le32 pages_per_block;
> > + __le32 blocks_per_lun;
> > + u8 lun_count;
> > + u8 addr_cycles;
> > + u8 bits_per_cell;
> > + __le16 bb_per_lun;
> > + __le16 block_endurance;
> > + u8 guaranteed_good_blocks;
> > + __le16 guaranteed_block_endurance;
> > + u8 programs_per_page;
> > + u8 ppage_attr;
> > + u8 ecc_bits;
> > + u8 interleaved_bits;
> > + u8 interleaved_ops;
> > + u8 reserved2[13];
> > +
> > + /* electrical parameter block */
> > + u8 io_pin_capacitance_max;
> > + __le16 async_timing_mode;
> > + __le16 program_cache_timing_mode;
> > + __le16 t_prog;
> > + __le16 t_bers;
> > + __le16 t_r;
> > + __le16 t_ccs;
> > + u8 reserved3[23];
> > +
> > + /* vendor */
> > + __le16 vendor_revision;
> > + u8 vendor_specific[14];
> > + u8 reserved4[68];
> > + u8 ecc_max_correct_ability;
> > + u8 die_select_feature;
> > + u8 reserved5[4];
> > +
> > + __le16 crc;
> > +} __packed;
> > +
>
> Please use the nand_onfi_params definition (include/linux/mtd/onfi.h).
>
> > +#endif /* __MICRON_H */
>
>
> Regards,
>
> Boris
Hello,
"Shivamurthy Shastri (sshivamurthy)" <[email protected]> wrote on
Mon, 4 Mar 2019 13:29:21 +0000:
> Hi Boris,
>
> > -----Original Message-----
> > From: Boris Brezillon <[email protected]>
> > Sent: Monday, February 4, 2019 7:02 PM
> > To: Shivamurthy Shastri (sshivamurthy) <[email protected]>
> > Cc: Miquel Raynal <[email protected]>; linux-
> > [email protected]; [email protected]; Chuanhong Guo
> > <[email protected]>; Richard Weinberger <[email protected]>; Schrempf
> > Frieder <[email protected]>; Marek Vasut
> > <[email protected]>; Frieder Schrempf
> > <[email protected]>; Brian Norris
> > <[email protected]>; David Woodhouse
> > <[email protected]>; Bean Huo (beanhuo) <[email protected]>
> > Subject: [EXT] Re: [PATCH 2/2] mtd: spinand: micron: Support for all Micron
> > SPI NAND flashes
> >
> > Hi Shivamurthy,
> >
> > On Mon, 4 Feb 2019 11:17:51 +0000
> > "Shivamurthy Shastri (sshivamurthy)" <[email protected]> wrote:
> >
> > > Driver is redesigned using parameter page to support all the Micron
> > > SPI NAND flashes.
> >
> > Do all Micron SPI NANDs really expose a valid ONFI param page? If
> > that's not the case, then relying on ONFi parsing only sounds like a
> > bad idea.
>
> Micron SPI NAND datasheet does not confirm to be as ONFI standard.
> However, they all expose parameter page, which I used for development.
>
> >
> > >
> > > Parameter page of Micron flashes is similar to ONFI parameter table and
> > > functionality is same, so copied some of the common functions like crc16
> > > and bit_wise_majority from nand_onfi.c.
> >
> > Most of the code is generic and does not depend on the spinand layer,
> > plus, we already have ONFI param page parsing code in
> > drivers/mtd/nand/raw/ which you're intentionally duplicating in a
> > version that will not be re-usable by the raw NAND layer even after
> > converting it to use the generic NAND layer.
> >
> > Please move ONFi parsing code to drivers/mtd/nand/onfi.c and make it
> > generic.
>
> As I said before, it is not compliant to ONFI standard, I think it is better not
> to make it generic.
For what I see it is too similar to copy all that code. I agree with
Boris. If there are some specificities that are not in the ONFI
standard you can do some late changes in the parameter page from
Micron's driver I guess?
Thanks,
Miquèl