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
