Re: [PATCH 2/2] mtd: spinand: micron: Support for all Micron SPI NAND flashes

From: Boris Brezillon
Date: Mon Feb 04 2019 - 13:02:02 EST

Hi Shivamurthy,

On Mon, 4 Feb 2019 11:17:51 +0000
"Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@xxxxxxxxxx> 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 <sshivamurthy@xxxxxxxxxx>
> Reviewed-by: Bean Huo <beanhuo@xxxxxxxxxx>

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 *)&params[i], 254) ==
> + le16_to_cpu(params->crc)) {
> + if (i)
> + memcpy(params, &params[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 <sshivamurthy@xxxxxxxxxx>
> + */
> +
> +#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