Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   "Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com>
To:     Boris Brezillon <bbrezillon@kernel.org>
CC:     Miquel Raynal <miquel.raynal@bootlin.com>,
        "linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Chuanhong Guo <gch981213@gmail.com>,
        "Richard Weinberger" <richard@nod.at>,
        Schrempf Frieder <frieder.schrempf@kontron.De>,
        Marek Vasut <marek.vasut@gmail.com>,
        Frieder Schrempf <frieder.schrempf@exceet.de>,
        Brian Norris <computersforpeace@gmail.com>,
        David Woodhouse <dwmw2@infradead.org>,
        "Bean Huo (beanhuo)" <beanhuo@micron.com>
Subject: RE: [EXT] Re: [PATCH 2/2] mtd: spinand: micron: Support for all
 Micron SPI NAND flashes
Thread-Topic: [EXT] Re: [PATCH 2/2] mtd: spinand: micron: Support for all
 Micron SPI NAND flashes
Thread-Index: AdS8exJYD8uSWbLRR5WRv3oHfltCMQAOKHqABXYK3+A=
Date:   Mon, 4 Mar 2019 13:29:21 +0000
Message-ID: <MN2PR08MB5951EE4ACB78BAD12EDA5F76B8710@MN2PR08MB5951.namprd08.prod.outlook.com>
References: <MN2PR08MB5951FD3B8244ABBA72FF7430B86D0@MN2PR08MB5951.namprd08.prod.outlook.com>
 <20190204190151.2c7987b7@bbrezillon>
In-Reply-To: <20190204190151.2c7987b7@bbrezillon>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: micron.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: e6beb2aa-a9a6-4693-11d2-08d6a0a56908
X-MS-Exchange-CrossTenant-originalarrivaltime: 04 Mar 2019 13:29:21.0667
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: f38a5ecd-2813-4862-b11b-ac1d563c806f
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR08MB5758
X-OriginatorOrg: micron.com
X-TM-AS-Product-Ver: SMEX-12.0.0.1782-8.200.1013-24468.005
X-TM-AS-Result: No--24.451200-0.000000-31
X-TM-AS-MatchedID: 150567-139010-139006-703523-704746-700648-851106-187165-7
        00022-700075-110462-708060-703712-121536-701702-706561-700038-708733-702198
        -701604-105640-706823-702865-702358-106230-704049-700287-701475-700016-7035
        29-188019-706290-847298-700324-702143-701384-700057-105630-702033-708712-70
        4473-700398-700264-851458-188199-700059-700492-706771-703399-703283-780052-
        700200-709403-701698-704156-700042-708797-700300-702898-702609-709584-70569
        4-704713-700251-704425-121336-702796-704407-121169-702037-711109-121119-113
        238-702091-707663-708558-148004-148007-148020-148050-20043-29997-42000-4200
        3-63-190014
X-TM-AS-User-Approved-Sender: Yes
X-TM-AS-User-Blocked-Sender: No
X-MT-CheckInternalSenderRule: True
X-Scanned-By: MIMEDefang 2.78 on 137.201.82.98
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Boris,

> -----Original Message-----
> From: Boris Brezillon <bbrezillon@kernel.org>
> Sent: Monday, February 4, 2019 7:02 PM
> To: Shivamurthy Shastri (sshivamurthy) <sshivamurthy@micron.com>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>; linux-
> mtd@lists.infradead.org; linux-kernel@vger.kernel.org; Chuanhong Guo
> <gch981213@gmail.com>; Richard Weinberger <richard@nod.at>; Schrempf
> Frieder <frieder.schrempf@kontron.De>; Marek Vasut
> <marek.vasut@gmail.com>; Frieder Schrempf
> <frieder.schrempf@exceet.de>; Brian Norris
> <computersforpeace@gmail.com>; David Woodhouse
> <dwmw2@infradead.org>; Bean Huo (beanhuo) <beanhuo@micron.com>
> Subject: [EXT] Re: [PATCH 2/2] mtd: spinand: micron: Support for all Micr=
on
> SPI NAND flashes
>=20
> Hi Shivamurthy,
>=20
> On Mon, 4 Feb 2019 11:17:51 +0000
> "Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com> wrote:
>=20
> > Driver is redesigned using parameter page to support all the Micron
> > SPI NAND flashes.
>=20
> 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.

>=20
> >
> > Parameter page of Micron flashes is similar to ONFI parameter table and
> > functionality is same, so copied some of the common functions like crc1=
6
> > and bit_wise_majority from nand_onfi.c.
>=20
> 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.
>=20
> 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 bette=
r not=20
to make it generic.

>=20
> >
> > This driver is tested using MT29F2G01ABXGD, MT29F4G01ABXFD,
> MT29F8G01ADXFD,
> > MT29F1G01ABXFD.
> >
> > Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> > Reviewed-by: Bean Huo <beanhuo@micron.com>
>=20
> I wish this code review had happened publicly.
>=20
> > ---
> >  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 =3D 64;
> > -	region->length =3D 64;
> > +	region->offset =3D mtd->oobsize / 2;
> > +	region->length =3D 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 =3D 2;
> > -	region->length =3D 62;
> > +	region->length =3D (mtd->oobsize / 2) - 2;
> >
> >  	return 0;
> >  }
> >
> > -static const struct mtd_ooblayout_ops mt29f2g01abagd_ooblayout =3D {
> > -	.ecc =3D mt29f2g01abagd_ooblayout_ecc,
> > -	.free =3D mt29f2g01abagd_ooblayout_free,
> > +static const struct mtd_ooblayout_ops ooblayout =3D {
> > +	.ecc =3D ooblayout_ecc,
> > +	.free =3D 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[] =3D {
> > -	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 ^=3D *p++ << 8;
> > +		for (i =3D 0; i < 8; i++)
> > +			crc =3D (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 =3D 0; i < bufsize; i++) {
> > +		u8 val =3D 0;
> > +
> > +		for (j =3D 0; j < 8; j++) {
> > +			unsigned int cnt =3D 0;
> > +
> > +			for (k =3D 0; k < nsrcbufs; k++) {
> > +				const u8 *srcbuf =3D srcbufs[k];
> > +
> > +				if (srcbuf[i] & BIT(j))
> > +					cnt++;
> > +			}
> > +
> > +			if (cnt > nsrcbufs / 2)
> > +				val |=3D BIT(j);
> > +		}
> > +
> > +		((u8 *)dstbuf)[i] =3D val;
> > +	}
> > +}
> >
> >  static int micron_spinand_detect(struct spinand_device *spinand)
> >  {
> > +	struct spinand_info deviceinfo;
> > +	struct micron_spinand_params *params;
> >  	u8 *id =3D 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] !=3D SPINAND_MFR_MICRON)
> >  		return 0;
> >
> > -	ret =3D spinand_match_and_init(spinand, micron_spinand_table,
> > -				     ARRAY_SIZE(micron_spinand_table),
> id[2]);
> > +	params =3D kzalloc(sizeof(*params) * 3, GFP_KERNEL);
> > +	if (!params)
> > +		return -ENOMEM;
> > +
> > +	ret =3D spinand_parameter_page_read(spinand, PARAMETER_PAGE,
> params,
> > +					  sizeof(*params) * 3);
> >  	if (ret)
> > -		return ret;
> > +		goto free_params;
> > +
> > +	for (i =3D 0; i < 3; i++) {
> > +		if (spinand_crc16(0x4F4E, (u8 *)&params[i], 254) =3D=3D
> > +				le16_to_cpu(params->crc)) {
> > +			if (i)
> > +				memcpy(params, &params[i],
> sizeof(*params));
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (i =3D=3D 3) {
> > +		const void *srcbufs[3] =3D {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) !=3D
> > +				le16_to_cpu(params->crc)) {
> > +			pr_err("Parameter page recovery failed,
> aborting\n");
> > +			goto free_params;
> > +		}
> > +	}
> > +
> > +	params->model[sizeof(params->model) - 1] =3D 0;
> > +	strim(params->model);
> > +
> > +	deviceinfo.model =3D kstrdup(params->model, GFP_KERNEL);
> > +	if (!deviceinfo.model) {
> > +		ret =3D -ENOMEM;
> > +		goto free_params;
> > +	}
> > +
> > +	deviceinfo.devid =3D id[2];
> > +	deviceinfo.flags =3D 0;
> > +	deviceinfo.memorg.bits_per_cell =3D params->bits_per_cell;
> > +	deviceinfo.memorg.pagesize =3D params->byte_per_page;
> > +	deviceinfo.memorg.oobsize =3D params->spare_bytes_per_page;
> > +	deviceinfo.memorg.pages_per_eraseblock =3D params-
> >pages_per_block;
> > +	deviceinfo.memorg.eraseblocks_per_lun =3D
> > +		params->blocks_per_lun * params->lun_count;
> > +	deviceinfo.memorg.planes_per_lun =3D params->lun_count;
>=20
> 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] .

>=20
> > +	deviceinfo.memorg.luns_per_target =3D 1;
> > +	deviceinfo.memorg.ntargets =3D 1;
> > +	deviceinfo.eccreq.strength =3D params->ecc_max_correct_ability;
> > +	deviceinfo.eccreq.step_size =3D 512;
> > +	deviceinfo.eccinfo.get_status =3D ecc_get_status;
> > +	deviceinfo.eccinfo.ooblayout =3D &ooblayout;
>=20
> Are all devices really using the same get_status method and the same
> layout. Sounds risky to me to assume this is the case.
>=20
> > +	deviceinfo.op_variants.read_cache =3D &read_cache_variants;
> > +	deviceinfo.op_variants.write_cache =3D &write_cache_variants;
> > +	deviceinfo.op_variants.update_cache =3D &update_cache_variants;
> > +
> > +	ret =3D spinand_match_and_init(spinand, &deviceinfo,
> > +				     1, id[2]);
>=20
> 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.
>=20
> > +	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 =3D {
> >  	.detect =3D micron_spinand_detect,
> > +	.init =3D micron_spinand_init,
> >  };
> >
> >  const struct spinand_manufacturer micron_spinand_manufacturer =3D {
> > 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@micron.com>
> > + */
> > +
> > +#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;
> > +
>=20
> Please use the nand_onfi_params definition (include/linux/mtd/onfi.h).
>=20
> > +#endif /* __MICRON_H */
>=20
>=20
> Regards,
>=20
> Boris