Hi Boris,
> -----Original Message-----
> From: Boris Brezillon <bbrezil...@kernel.org>
> Sent: Monday, February 4, 2019 7:02 PM
> To: Shivamurthy Shastri (sshivamurthy) <sshivamur...@micron.com>
> Cc: Miquel Raynal <miquel.ray...@bootlin.com>; linux-
> m...@lists.infradead.org; linux-kernel@vger.kernel.org; Chuanhong Guo
> <gch981...@gmail.com>; Richard Weinberger <rich...@nod.at>; Schrempf
> Frieder <frieder.schre...@kontron.de>; Marek Vasut
> <marek.va...@gmail.com>; Frieder Schrempf
> <frieder.schre...@exceet.de>; Brian Norris
> <computersforpe...@gmail.com>; David Woodhouse
> <dw...@infradead.org>; Bean Huo (beanhuo) <bean...@micron.com>
> 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)" <sshivamur...@micron.com> 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 <sshivamur...@micron.com>
> > Reviewed-by: Bean Huo <bean...@micron.com>
>
> 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 <sshivamur...@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;
> > +
>
> Please use the nand_onfi_params definition (include/linux/mtd/onfi.h).
>
> > +#endif /* __MICRON_H */
>
>
> Regards,
>
> Boris
