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

[Shivamurthy Shastri (sshivamurthy)]

Hi Boris,

> -----Original Message-----
> From: Boris Brezillon <bbrezillon@xxxxxxxxxx>
> Sent: Monday, February 4, 2019 7:02 PM
> To: Shivamurthy Shastri (sshivamurthy) <sshivamurthy@xxxxxxxxxx>
> Cc: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>; linux-
> mtd@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Chuanhong Guo
> <gch981213@xxxxxxxxx>; Richard Weinberger <richard@xxxxxx>; Schrempf
> Frieder <frieder.schrempf@xxxxxxxxxx>; Marek Vasut
> <marek.vasut@xxxxxxxxx>; Frieder Schrempf
> <frieder.schrempf@xxxxxxxxx>; Brian Norris
> <computersforpeace@xxxxxxxxx>; David Woodhouse
> <dwmw2@xxxxxxxxxxxxx>; Bean Huo (beanhuo) <beanhuo@xxxxxxxxxx>
> 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)" <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.

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 <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.

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 <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