| Message ID | 1541665796-21092-1-git-send-email-frieder.schrempf@kontron.de |
|---|---|
| State | Accepted |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | mtd: nand: spi: Add initial support for Toshiba TC58CVG2S0H | expand |
Nitpick: subject prefix should be "mtd: spinand" not "mtd: nand: spi" :-). On Thu, 8 Nov 2018 08:32:11 +0000 Schrempf Frieder <frieder.schrempf@kontron.De> wrote: > Add minimal support for the Toshiba TC58CVG2S0H SPI NAND chip. > > Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de> > --- > drivers/mtd/nand/spi/Makefile | 2 +- > drivers/mtd/nand/spi/core.c | 1 + > drivers/mtd/nand/spi/toshiba.c | 136 ++++++++++++++++++++++++++++++++++++ > include/linux/mtd/spinand.h | 1 + > 4 files changed, 139 insertions(+), 1 deletion(-) > > diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile > index b74e074..be5f735 100644 > --- a/drivers/mtd/nand/spi/Makefile > +++ b/drivers/mtd/nand/spi/Makefile > @@ -1,3 +1,3 @@ > # SPDX-License-Identifier: GPL-2.0 > -spinand-objs := core.o macronix.o micron.o winbond.o > +spinand-objs := core.o macronix.o micron.o toshiba.o winbond.o > obj-$(CONFIG_MTD_SPI_NAND) += spinand.o > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c > index 30f8364..87bdf2a 100644 > --- a/drivers/mtd/nand/spi/core.c > +++ b/drivers/mtd/nand/spi/core.c > @@ -766,6 +766,7 @@ static const struct nand_ops spinand_ops = { > static const struct spinand_manufacturer *spinand_manufacturers[] = { > ¯onix_spinand_manufacturer, > µn_spinand_manufacturer, > + &toshiba_spinand_manufacturer, > &winbond_spinand_manufacturer, > }; > > diff --git a/drivers/mtd/nand/spi/toshiba.c b/drivers/mtd/nand/spi/toshiba.c > new file mode 100644 > index 0000000..294bcf6 > --- /dev/null > +++ b/drivers/mtd/nand/spi/toshiba.c > @@ -0,0 +1,136 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2018 exceet electronics GmbH > + * Copyright (c) 2018 Kontron Electronics GmbH > + * > + * Author: Frieder Schrempf <frieder.schrempf@kontron.de> > + */ > + > +#include <linux/device.h> > +#include <linux/kernel.h> > +#include <linux/mtd/spinand.h> > + > +#define SPINAND_MFR_TOSHIBA 0x98 > + > +static SPINAND_OP_VARIANTS(read_cache_variants, > + SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0), > + SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0), > + SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0), > + SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0)); > + > +static SPINAND_OP_VARIANTS(write_cache_variants, > + SPINAND_PROG_LOAD(true, 0, NULL, 0)); > + > +static SPINAND_OP_VARIANTS(update_cache_variants, > + SPINAND_PROG_LOAD(false, 0, NULL, 0)); > + > +static int tc58cvg2s0h_ooblayout_ecc(struct mtd_info *mtd, int section, > + struct mtd_oob_region *region) > +{ > + if (section > 7) > + return -ERANGE; > + > + region->offset = 128 + 16 * section; > + region->length = 16; > + > + > + return 0; > +} > + > +static int tc58cvg2s0h_ooblayout_free(struct mtd_info *mtd, int section, > + struct mtd_oob_region *region) > +{ > + if (section > 7) > + return -ERANGE; > + > + region->offset = 2 + 16 * section; > + region->length = 14; > + > + > + return 0; > +} > + > +static const struct mtd_ooblayout_ops tc58cvg2s0h_ooblayout = { > + .ecc = tc58cvg2s0h_ooblayout_ecc, > + .free = tc58cvg2s0h_ooblayout_free, > +}; > + > +static int tc58cvg2s0h_ecc_get_status(struct spinand_device *spinand, > + u8 status) > +{ > + struct nand_device *nand = spinand_to_nand(spinand); > + u8 mbf = 0; > + struct spi_mem_op op = SPINAND_GET_FEATURE_OP(0x30, &mbf); > + > + switch (status & STATUS_ECC_MASK) { > + case STATUS_ECC_NO_BITFLIPS: > + return 0; > + > + case STATUS_ECC_UNCOR_ERROR: > + return -EBADMSG; > + > + case STATUS_ECC_HAS_BITFLIPS: > + /* > + * Let's try to retrieve the real maximum number of bitflips > + * in order to avoid forcing the wear-leveling layer to move > + * data around if it's not necessary. > + */ > + if (spi_mem_exec_op(spinand->spimem, &op)) > + return nand->eccreq.strength; > + > + mbf >>= 4; > + > + if (WARN_ON(mbf > nand->eccreq.strength || !mbf)) > + return nand->eccreq.strength; > + > + return mbf; > + > + default: > + break; > + } > + > + return -EINVAL; > +} > + > +static const struct spinand_info toshiba_spinand_table[] = { > + SPINAND_INFO("TC58CVG2S0H", 0xCD, > + NAND_MEMORG(1, 4096, 256, 64, 2048, 1, 1, 1), > + NAND_ECCREQ(8, 512), > + SPINAND_INFO_OP_VARIANTS(&read_cache_variants, > + &write_cache_variants, > + &update_cache_variants), > + SPINAND_HAS_QE_BIT, > + SPINAND_ECCINFO(&tc58cvg2s0h_ooblayout, > + tc58cvg2s0h_ecc_get_status)), > +}; > + > +static int toshiba_spinand_detect(struct spinand_device *spinand) > +{ > + u8 *id = spinand->id.data; > + int ret; > + > + /* > + * Toshiba SPI NAND read ID needs a dummy byte, > + * so the first byte in id is garbage. > + */ > + if (id[1] != SPINAND_MFR_TOSHIBA) > + return 0; > + > + ret = spinand_match_and_init(spinand, toshiba_spinand_table, > + ARRAY_SIZE(toshiba_spinand_table), > + id[2]); > + if (ret) > + return ret; > + > + return 1; > +} > + > +static const struct spinand_manufacturer_ops toshiba_spinand_manuf_ops = { > + .detect = toshiba_spinand_detect, > +}; > + > +const struct spinand_manufacturer toshiba_spinand_manufacturer = { > + .id = SPINAND_MFR_TOSHIBA, > + .name = "Toshiba", > + .ops = &toshiba_spinand_manuf_ops, > +}; > diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h > index 088ff96..816c4b0 100644 > --- a/include/linux/mtd/spinand.h > +++ b/include/linux/mtd/spinand.h > @@ -196,6 +196,7 @@ struct spinand_manufacturer { > /* SPI NAND manufacturers */ > extern const struct spinand_manufacturer macronix_spinand_manufacturer; > extern const struct spinand_manufacturer micron_spinand_manufacturer; > +extern const struct spinand_manufacturer toshiba_spinand_manufacturer; > extern const struct spinand_manufacturer winbond_spinand_manufacturer; > > /**
Hi Schrempf, Schrempf Frieder <frieder.schrempf@kontron.De> wrote on Thu, 8 Nov 2018 08:32:11 +0000: > Add minimal support for the Toshiba TC58CVG2S0H SPI NAND chip. > > Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de> > --- With "mtd: spinand:" as prefix, applied to nand/next. Thanks, Miquèl
+Clément Péron Hi Clément, FYI, this has already been merged to nand/next. Regards, Frieder On 08.11.18 09:29, Frieder Schrempf wrote: > Add minimal support for the Toshiba TC58CVG2S0H SPI NAND chip. > > Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de> > --- > drivers/mtd/nand/spi/Makefile | 2 +- > drivers/mtd/nand/spi/core.c | 1 + > drivers/mtd/nand/spi/toshiba.c | 136 ++++++++++++++++++++++++++++++++++++ > include/linux/mtd/spinand.h | 1 + > 4 files changed, 139 insertions(+), 1 deletion(-) > > diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile > index b74e074..be5f735 100644 > --- a/drivers/mtd/nand/spi/Makefile > +++ b/drivers/mtd/nand/spi/Makefile > @@ -1,3 +1,3 @@ > # SPDX-License-Identifier: GPL-2.0 > -spinand-objs := core.o macronix.o micron.o winbond.o > +spinand-objs := core.o macronix.o micron.o toshiba.o winbond.o > obj-$(CONFIG_MTD_SPI_NAND) += spinand.o > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c > index 30f8364..87bdf2a 100644 > --- a/drivers/mtd/nand/spi/core.c > +++ b/drivers/mtd/nand/spi/core.c > @@ -766,6 +766,7 @@ static const struct nand_ops spinand_ops = { > static const struct spinand_manufacturer *spinand_manufacturers[] = { > ¯onix_spinand_manufacturer, > µn_spinand_manufacturer, > + &toshiba_spinand_manufacturer, > &winbond_spinand_manufacturer, > }; > > diff --git a/drivers/mtd/nand/spi/toshiba.c b/drivers/mtd/nand/spi/toshiba.c > new file mode 100644 > index 0000000..294bcf6 > --- /dev/null > +++ b/drivers/mtd/nand/spi/toshiba.c > @@ -0,0 +1,136 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2018 exceet electronics GmbH > + * Copyright (c) 2018 Kontron Electronics GmbH > + * > + * Author: Frieder Schrempf <frieder.schrempf@kontron.de> > + */ > + > +#include <linux/device.h> > +#include <linux/kernel.h> > +#include <linux/mtd/spinand.h> > + > +#define SPINAND_MFR_TOSHIBA 0x98 > + > +static SPINAND_OP_VARIANTS(read_cache_variants, > + SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0), > + SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0), > + SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0), > + SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0)); > + > +static SPINAND_OP_VARIANTS(write_cache_variants, > + SPINAND_PROG_LOAD(true, 0, NULL, 0)); > + > +static SPINAND_OP_VARIANTS(update_cache_variants, > + SPINAND_PROG_LOAD(false, 0, NULL, 0)); > + > +static int tc58cvg2s0h_ooblayout_ecc(struct mtd_info *mtd, int section, > + struct mtd_oob_region *region) > +{ > + if (section > 7) > + return -ERANGE; > + > + region->offset = 128 + 16 * section; > + region->length = 16; > + > + > + return 0; > +} > + > +static int tc58cvg2s0h_ooblayout_free(struct mtd_info *mtd, int section, > + struct mtd_oob_region *region) > +{ > + if (section > 7) > + return -ERANGE; > + > + region->offset = 2 + 16 * section; > + region->length = 14; > + > + > + return 0; > +} > + > +static const struct mtd_ooblayout_ops tc58cvg2s0h_ooblayout = { > + .ecc = tc58cvg2s0h_ooblayout_ecc, > + .free = tc58cvg2s0h_ooblayout_free, > +}; > + > +static int tc58cvg2s0h_ecc_get_status(struct spinand_device *spinand, > + u8 status) > +{ > + struct nand_device *nand = spinand_to_nand(spinand); > + u8 mbf = 0; > + struct spi_mem_op op = SPINAND_GET_FEATURE_OP(0x30, &mbf); > + > + switch (status & STATUS_ECC_MASK) { > + case STATUS_ECC_NO_BITFLIPS: > + return 0; > + > + case STATUS_ECC_UNCOR_ERROR: > + return -EBADMSG; > + > + case STATUS_ECC_HAS_BITFLIPS: > + /* > + * Let's try to retrieve the real maximum number of bitflips > + * in order to avoid forcing the wear-leveling layer to move > + * data around if it's not necessary. > + */ > + if (spi_mem_exec_op(spinand->spimem, &op)) > + return nand->eccreq.strength; > + > + mbf >>= 4; > + > + if (WARN_ON(mbf > nand->eccreq.strength || !mbf)) > + return nand->eccreq.strength; > + > + return mbf; > + > + default: > + break; > + } > + > + return -EINVAL; > +} > + > +static const struct spinand_info toshiba_spinand_table[] = { > + SPINAND_INFO("TC58CVG2S0H", 0xCD, > + NAND_MEMORG(1, 4096, 256, 64, 2048, 1, 1, 1), > + NAND_ECCREQ(8, 512), > + SPINAND_INFO_OP_VARIANTS(&read_cache_variants, > + &write_cache_variants, > + &update_cache_variants), > + SPINAND_HAS_QE_BIT, > + SPINAND_ECCINFO(&tc58cvg2s0h_ooblayout, > + tc58cvg2s0h_ecc_get_status)), > +}; > + > +static int toshiba_spinand_detect(struct spinand_device *spinand) > +{ > + u8 *id = spinand->id.data; > + int ret; > + > + /* > + * Toshiba SPI NAND read ID needs a dummy byte, > + * so the first byte in id is garbage. > + */ > + if (id[1] != SPINAND_MFR_TOSHIBA) > + return 0; > + > + ret = spinand_match_and_init(spinand, toshiba_spinand_table, > + ARRAY_SIZE(toshiba_spinand_table), > + id[2]); > + if (ret) > + return ret; > + > + return 1; > +} > + > +static const struct spinand_manufacturer_ops toshiba_spinand_manuf_ops = { > + .detect = toshiba_spinand_detect, > +}; > + > +const struct spinand_manufacturer toshiba_spinand_manufacturer = { > + .id = SPINAND_MFR_TOSHIBA, > + .name = "Toshiba", > + .ops = &toshiba_spinand_manuf_ops, > +}; > diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h > index 088ff96..816c4b0 100644 > --- a/include/linux/mtd/spinand.h > +++ b/include/linux/mtd/spinand.h > @@ -196,6 +196,7 @@ struct spinand_manufacturer { > /* SPI NAND manufacturers */ > extern const struct spinand_manufacturer macronix_spinand_manufacturer; > extern const struct spinand_manufacturer micron_spinand_manufacturer; > +extern const struct spinand_manufacturer toshiba_spinand_manufacturer; > extern const struct spinand_manufacturer winbond_spinand_manufacturer; > > /** >
Hi Frieder, On Tue, 27 Nov 2018 at 16:08, Schrempf Frieder <frieder.schrempf@kontron.de> wrote: > > +Clément Péron > > Hi Clément, > > FYI, this has already been merged to nand/next. Just want to point the difference that I made when I try to introduce my driver. The datasheet I used is this one : https://media.digikey.com/pdf/Data%20Sheets/Toshiba%20PDFs/TC58CVG2S0HxAIx_Rev1.1_2016-11-08.pdf > > Regards, > Frieder > > On 08.11.18 09:29, Frieder Schrempf wrote: > > Add minimal support for the Toshiba TC58CVG2S0H SPI NAND chip. > > > > Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de> > > --- > > drivers/mtd/nand/spi/Makefile | 2 +- > > drivers/mtd/nand/spi/core.c | 1 + > > drivers/mtd/nand/spi/toshiba.c | 136 ++++++++++++++++++++++++++++++++++++ > > include/linux/mtd/spinand.h | 1 + > > 4 files changed, 139 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile > > index b74e074..be5f735 100644 > > --- a/drivers/mtd/nand/spi/Makefile > > +++ b/drivers/mtd/nand/spi/Makefile > > @@ -1,3 +1,3 @@ > > # SPDX-License-Identifier: GPL-2.0 > > -spinand-objs := core.o macronix.o micron.o winbond.o > > +spinand-objs := core.o macronix.o micron.o toshiba.o winbond.o > > obj-$(CONFIG_MTD_SPI_NAND) += spinand.o > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c > > index 30f8364..87bdf2a 100644 > > --- a/drivers/mtd/nand/spi/core.c > > +++ b/drivers/mtd/nand/spi/core.c > > @@ -766,6 +766,7 @@ static const struct nand_ops spinand_ops = { > > static const struct spinand_manufacturer *spinand_manufacturers[] = { > > ¯onix_spinand_manufacturer, > > µn_spinand_manufacturer, > > + &toshiba_spinand_manufacturer, > > &winbond_spinand_manufacturer, > > }; > > > > diff --git a/drivers/mtd/nand/spi/toshiba.c b/drivers/mtd/nand/spi/toshiba.c > > new file mode 100644 > > index 0000000..294bcf6 > > --- /dev/null > > +++ b/drivers/mtd/nand/spi/toshiba.c > > @@ -0,0 +1,136 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (c) 2018 exceet electronics GmbH > > + * Copyright (c) 2018 Kontron Electronics GmbH > > + * > > + * Author: Frieder Schrempf <frieder.schrempf@kontron.de> > > + */ > > + > > +#include <linux/device.h> > > +#include <linux/kernel.h> > > +#include <linux/mtd/spinand.h> > > + > > +#define SPINAND_MFR_TOSHIBA 0x98 > > + > > +static SPINAND_OP_VARIANTS(read_cache_variants, > > + SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0), > > + SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0), > > + SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0), > > + SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0)); > > + > > +static SPINAND_OP_VARIANTS(write_cache_variants, > > + SPINAND_PROG_LOAD(true, 0, NULL, 0)); > > + > > +static SPINAND_OP_VARIANTS(update_cache_variants, > > + SPINAND_PROG_LOAD(false, 0, NULL, 0)); > > + > > +static int tc58cvg2s0h_ooblayout_ecc(struct mtd_info *mtd, int section, > > + struct mtd_oob_region *region) > > +{ > > + if (section > 7) > > + return -ERANGE; > > + > > + region->offset = 128 + 16 * section; > > + region->length = 16; Here you expose the ECC bits has 8 sections of 16 bytes. But regarding the datasheet this should not be accessed page 32. "The ECC parity code generated by internal ECC is stored in column addresses 4224-4351 and the users cannot access to these specific addresses when internal ECC is enabled." > > + > > + > > + return 0; > > +} > > + > > +static int tc58cvg2s0h_ooblayout_free(struct mtd_info *mtd, int section, > > + struct mtd_oob_region *region) > > +{ > > + if (section > 7) > > + return -ERANGE; > > + > > + region->offset = 2 + 16 * section; > > + region->length = 14; This reserved 2 bytes for BBM for each section. But maybe we could declare this as 1 section of 128bytes: if (section) return -ERANGE; region->offset = 2; region->length = 126; > > + > > + > > + return 0; > > +} > > + > > +static const struct mtd_ooblayout_ops tc58cvg2s0h_ooblayout = { > > + .ecc = tc58cvg2s0h_ooblayout_ecc, > > + .free = tc58cvg2s0h_ooblayout_free, > > +}; > > + > > +static int tc58cvg2s0h_ecc_get_status(struct spinand_device *spinand, > > + u8 status) > > +{ > > + struct nand_device *nand = spinand_to_nand(spinand); > > + u8 mbf = 0; > > + struct spi_mem_op op = SPINAND_GET_FEATURE_OP(0x30, &mbf); > > + > > + switch (status & STATUS_ECC_MASK) { > > + case STATUS_ECC_NO_BITFLIPS: > > + return 0; > > + > > + case STATUS_ECC_UNCOR_ERROR: > > + return -EBADMSG; > > + > > + case STATUS_ECC_HAS_BITFLIPS: > > + /* > > + * Let's try to retrieve the real maximum number of bitflips > > + * in order to avoid forcing the wear-leveling layer to move > > + * data around if it's not necessary. > > + */ > > + if (spi_mem_exec_op(spinand->spimem, &op)) > > + return nand->eccreq.strength; > > + > > + mbf >>= 4; > > + > > + if (WARN_ON(mbf > nand->eccreq.strength || !mbf)) > > + return nand->eccreq.strength; > > + > > + return mbf; > > + These take take care of 0, 0x1 and 0x2 but not 0x3 which is valid page 26 of the datasheet : ECC status bits indicate the status of internal ECC operation 00b: No bit flips were detected in previous page read. 01b: Bit flips were detected and corrected. Bit flip count did not exceed the bit flip detection threshold. The threshold is set by bits [7:4] in address 10h in the feature table. 10b: Multiple bit flips were detected and not corrected. 11b: Bit flips were detected and corrected. Bit flip count exceeded the bit flip detection threshold. The threshold is set by bits [7:4] in address 10h in the feature table Regards, Clement > > + default: > > + break; > > + } > > + > > + return -EINVAL; > > +} > > + > > +static const struct spinand_info toshiba_spinand_table[] = { > > + SPINAND_INFO("TC58CVG2S0H", 0xCD, > > + NAND_MEMORG(1, 4096, 256, 64, 2048, 1, 1, 1), > > + NAND_ECCREQ(8, 512), > > + SPINAND_INFO_OP_VARIANTS(&read_cache_variants, > > + &write_cache_variants, > > + &update_cache_variants), > > + SPINAND_HAS_QE_BIT, > > + SPINAND_ECCINFO(&tc58cvg2s0h_ooblayout, > > + tc58cvg2s0h_ecc_get_status)), > > +}; > > + > > +static int toshiba_spinand_detect(struct spinand_device *spinand) > > +{ > > + u8 *id = spinand->id.data; > > + int ret; > > + > > + /* > > + * Toshiba SPI NAND read ID needs a dummy byte, > > + * so the first byte in id is garbage. > > + */ > > + if (id[1] != SPINAND_MFR_TOSHIBA) > > + return 0; > > + > > + ret = spinand_match_and_init(spinand, toshiba_spinand_table, > > + ARRAY_SIZE(toshiba_spinand_table), > > + id[2]); > > + if (ret) > > + return ret; > > + > > + return 1; > > +} > > + > > +static const struct spinand_manufacturer_ops toshiba_spinand_manuf_ops = { > > + .detect = toshiba_spinand_detect, > > +}; > > + > > +const struct spinand_manufacturer toshiba_spinand_manufacturer = { > > + .id = SPINAND_MFR_TOSHIBA, > > + .name = "Toshiba", > > + .ops = &toshiba_spinand_manuf_ops, > > +}; > > diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h > > index 088ff96..816c4b0 100644 > > --- a/include/linux/mtd/spinand.h > > +++ b/include/linux/mtd/spinand.h > > @@ -196,6 +196,7 @@ struct spinand_manufacturer { > > /* SPI NAND manufacturers */ > > extern const struct spinand_manufacturer macronix_spinand_manufacturer; > > extern const struct spinand_manufacturer micron_spinand_manufacturer; > > +extern const struct spinand_manufacturer toshiba_spinand_manufacturer; > > extern const struct spinand_manufacturer winbond_spinand_manufacturer; > > > > /** > >
Hi Clément, On 27.11.18 16:18, Clément Péron wrote: > Hi Frieder, > > On Tue, 27 Nov 2018 at 16:08, Schrempf Frieder > <frieder.schrempf@kontron.de> wrote: >> >> +Clément Péron >> >> Hi Clément, >> >> FYI, this has already been merged to nand/next. > Just want to point the difference that I made when I try to introduce my driver. > The datasheet I used is this one : > https://media.digikey.com/pdf/Data%20Sheets/Toshiba%20PDFs/TC58CVG2S0HxAIx_Rev1.1_2016-11-08.pdf > >> >> Regards, >> Frieder >> >> On 08.11.18 09:29, Frieder Schrempf wrote: >>> Add minimal support for the Toshiba TC58CVG2S0H SPI NAND chip. >>> >>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de> >>> --- >>> drivers/mtd/nand/spi/Makefile | 2 +- >>> drivers/mtd/nand/spi/core.c | 1 + >>> drivers/mtd/nand/spi/toshiba.c | 136 ++++++++++++++++++++++++++++++++++++ >>> include/linux/mtd/spinand.h | 1 + >>> 4 files changed, 139 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile >>> index b74e074..be5f735 100644 >>> --- a/drivers/mtd/nand/spi/Makefile >>> +++ b/drivers/mtd/nand/spi/Makefile >>> @@ -1,3 +1,3 @@ >>> # SPDX-License-Identifier: GPL-2.0 >>> -spinand-objs := core.o macronix.o micron.o winbond.o >>> +spinand-objs := core.o macronix.o micron.o toshiba.o winbond.o >>> obj-$(CONFIG_MTD_SPI_NAND) += spinand.o >>> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c >>> index 30f8364..87bdf2a 100644 >>> --- a/drivers/mtd/nand/spi/core.c >>> +++ b/drivers/mtd/nand/spi/core.c >>> @@ -766,6 +766,7 @@ static const struct nand_ops spinand_ops = { >>> static const struct spinand_manufacturer *spinand_manufacturers[] = { >>> ¯onix_spinand_manufacturer, >>> µn_spinand_manufacturer, >>> + &toshiba_spinand_manufacturer, >>> &winbond_spinand_manufacturer, >>> }; >>> >>> diff --git a/drivers/mtd/nand/spi/toshiba.c b/drivers/mtd/nand/spi/toshiba.c >>> new file mode 100644 >>> index 0000000..294bcf6 >>> --- /dev/null >>> +++ b/drivers/mtd/nand/spi/toshiba.c >>> @@ -0,0 +1,136 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * Copyright (c) 2018 exceet electronics GmbH >>> + * Copyright (c) 2018 Kontron Electronics GmbH >>> + * >>> + * Author: Frieder Schrempf <frieder.schrempf@kontron.de> >>> + */ >>> + >>> +#include <linux/device.h> >>> +#include <linux/kernel.h> >>> +#include <linux/mtd/spinand.h> >>> + >>> +#define SPINAND_MFR_TOSHIBA 0x98 >>> + >>> +static SPINAND_OP_VARIANTS(read_cache_variants, >>> + SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0), >>> + SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0), >>> + SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0), >>> + SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0)); >>> + >>> +static SPINAND_OP_VARIANTS(write_cache_variants, >>> + SPINAND_PROG_LOAD(true, 0, NULL, 0)); >>> + >>> +static SPINAND_OP_VARIANTS(update_cache_variants, >>> + SPINAND_PROG_LOAD(false, 0, NULL, 0)); >>> + >>> +static int tc58cvg2s0h_ooblayout_ecc(struct mtd_info *mtd, int section, >>> + struct mtd_oob_region *region) >>> +{ >>> + if (section > 7) >>> + return -ERANGE; >>> + >>> + region->offset = 128 + 16 * section; >>> + region->length = 16; > > Here you expose the ECC bits has 8 sections of 16 bytes. > But regarding the datasheet this should not be accessed page 32. > "The ECC parity code generated by internal ECC is stored in column > addresses 4224-4351 and the users cannot access to these specific > addresses when internal ECC is enabled." This is just to let the other layers know, where the bytes used for ECC are. As long as the driver uses the on-chip ECC it won't write to this area. So this is correct unless I misunderstood this concept. All the other supported SPI NAND chips use the same approach. > >>> + >>> + >>> + return 0; >>> +} >>> + >>> +static int tc58cvg2s0h_ooblayout_free(struct mtd_info *mtd, int section, >>> + struct mtd_oob_region *region) >>> +{ >>> + if (section > 7) >>> + return -ERANGE; >>> + >>> + region->offset = 2 + 16 * section; >>> + region->length = 14; > > This reserved 2 bytes for BBM for each section. > But maybe we could declare this as 1 section of 128bytes: > > if (section) > return -ERANGE; > > region->offset = 2; > region->length = 126; The datasheet (p. 32) describes that the OOB data of a page is divided into 8 sections. So why should we not reflect this in the software? Probably it would be sufficient to reserve two bytes for the bad block marker at the beginning, instead of two bytes in each section. But I'm not sure about this and it doesn't really hurt. > >>> + >>> + >>> + return 0; >>> +} >>> + >>> +static const struct mtd_ooblayout_ops tc58cvg2s0h_ooblayout = { >>> + .ecc = tc58cvg2s0h_ooblayout_ecc, >>> + .free = tc58cvg2s0h_ooblayout_free, >>> +}; >>> + >>> +static int tc58cvg2s0h_ecc_get_status(struct spinand_device *spinand, >>> + u8 status) >>> +{ >>> + struct nand_device *nand = spinand_to_nand(spinand); >>> + u8 mbf = 0; >>> + struct spi_mem_op op = SPINAND_GET_FEATURE_OP(0x30, &mbf); >>> + >>> + switch (status & STATUS_ECC_MASK) { >>> + case STATUS_ECC_NO_BITFLIPS: >>> + return 0; >>> + >>> + case STATUS_ECC_UNCOR_ERROR: >>> + return -EBADMSG; >>> + >>> + case STATUS_ECC_HAS_BITFLIPS: >>> + /* >>> + * Let's try to retrieve the real maximum number of bitflips >>> + * in order to avoid forcing the wear-leveling layer to move >>> + * data around if it's not necessary. >>> + */ >>> + if (spi_mem_exec_op(spinand->spimem, &op)) >>> + return nand->eccreq.strength; >>> + >>> + mbf >>= 4; >>> + >>> + if (WARN_ON(mbf > nand->eccreq.strength || !mbf)) >>> + return nand->eccreq.strength; >>> + >>> + return mbf; >>> + > > These take take care of 0, 0x1 and 0x2 but not 0x3 which is valid Hm, you're right. We don't care if it is 0x1 or 0x3 (both mean that bitflips were corrected), but we currently only check for 0x1. I will send a fix for this tomorrow. Thanks, Frieder > page 26 of the datasheet : > > ECC status bits indicate the status of internal ECC operation > 00b: No bit flips were detected in previous page read. > 01b: Bit flips were detected and corrected. Bit flip count did not > exceed the bit flip detection threshold. The threshold is set by bits > [7:4] in address 10h in the feature table. > 10b: Multiple bit flips were detected and not corrected. > 11b: Bit flips were detected and corrected. Bit flip count exceeded > the bit flip detection threshold. The threshold is set by bits [7:4] > in address 10h in the feature table > > Regards, > Clement > >>> + default: >>> + break; >>> + } >>> + >>> + return -EINVAL; >>> +} >>> + >>> +static const struct spinand_info toshiba_spinand_table[] = { >>> + SPINAND_INFO("TC58CVG2S0H", 0xCD, >>> + NAND_MEMORG(1, 4096, 256, 64, 2048, 1, 1, 1), >>> + NAND_ECCREQ(8, 512), >>> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants, >>> + &write_cache_variants, >>> + &update_cache_variants), >>> + SPINAND_HAS_QE_BIT, >>> + SPINAND_ECCINFO(&tc58cvg2s0h_ooblayout, >>> + tc58cvg2s0h_ecc_get_status)), >>> +}; >>> + >>> +static int toshiba_spinand_detect(struct spinand_device *spinand) >>> +{ >>> + u8 *id = spinand->id.data; >>> + int ret; >>> + >>> + /* >>> + * Toshiba SPI NAND read ID needs a dummy byte, >>> + * so the first byte in id is garbage. >>> + */ >>> + if (id[1] != SPINAND_MFR_TOSHIBA) >>> + return 0; >>> + >>> + ret = spinand_match_and_init(spinand, toshiba_spinand_table, >>> + ARRAY_SIZE(toshiba_spinand_table), >>> + id[2]); >>> + if (ret) >>> + return ret; >>> + >>> + return 1; >>> +} >>> + >>> +static const struct spinand_manufacturer_ops toshiba_spinand_manuf_ops = { >>> + .detect = toshiba_spinand_detect, >>> +}; >>> + >>> +const struct spinand_manufacturer toshiba_spinand_manufacturer = { >>> + .id = SPINAND_MFR_TOSHIBA, >>> + .name = "Toshiba", >>> + .ops = &toshiba_spinand_manuf_ops, >>> +}; >>> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h >>> index 088ff96..816c4b0 100644 >>> --- a/include/linux/mtd/spinand.h >>> +++ b/include/linux/mtd/spinand.h >>> @@ -196,6 +196,7 @@ struct spinand_manufacturer { >>> /* SPI NAND manufacturers */ >>> extern const struct spinand_manufacturer macronix_spinand_manufacturer; >>> extern const struct spinand_manufacturer micron_spinand_manufacturer; >>> +extern const struct spinand_manufacturer toshiba_spinand_manufacturer; >>> extern const struct spinand_manufacturer winbond_spinand_manufacturer; >>> >>> /** >>>
Hi Frieder, On Tue, 27 Nov 2018 at 17:42, Schrempf Frieder <frieder.schrempf@kontron.de> wrote: > > Hi Clément, > > On 27.11.18 16:18, Clément Péron wrote: > > Hi Frieder, > > > > On Tue, 27 Nov 2018 at 16:08, Schrempf Frieder > > <frieder.schrempf@kontron.de> wrote: > >> > >> +Clément Péron > >> > >> Hi Clément, > >> > >> FYI, this has already been merged to nand/next. > > Just want to point the difference that I made when I try to introduce my driver. > > The datasheet I used is this one : > > https://media.digikey.com/pdf/Data%20Sheets/Toshiba%20PDFs/TC58CVG2S0HxAIx_Rev1.1_2016-11-08.pdf > > > >> > >> Regards, > >> Frieder > >> > >> On 08.11.18 09:29, Frieder Schrempf wrote: > >>> Add minimal support for the Toshiba TC58CVG2S0H SPI NAND chip. > >>> > >>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de> > >>> --- > >>> drivers/mtd/nand/spi/Makefile | 2 +- > >>> drivers/mtd/nand/spi/core.c | 1 + > >>> drivers/mtd/nand/spi/toshiba.c | 136 ++++++++++++++++++++++++++++++++++++ > >>> include/linux/mtd/spinand.h | 1 + > >>> 4 files changed, 139 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile > >>> index b74e074..be5f735 100644 > >>> --- a/drivers/mtd/nand/spi/Makefile > >>> +++ b/drivers/mtd/nand/spi/Makefile > >>> @@ -1,3 +1,3 @@ > >>> # SPDX-License-Identifier: GPL-2.0 > >>> -spinand-objs := core.o macronix.o micron.o winbond.o > >>> +spinand-objs := core.o macronix.o micron.o toshiba.o winbond.o > >>> obj-$(CONFIG_MTD_SPI_NAND) += spinand.o > >>> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c > >>> index 30f8364..87bdf2a 100644 > >>> --- a/drivers/mtd/nand/spi/core.c > >>> +++ b/drivers/mtd/nand/spi/core.c > >>> @@ -766,6 +766,7 @@ static const struct nand_ops spinand_ops = { > >>> static const struct spinand_manufacturer *spinand_manufacturers[] = { > >>> ¯onix_spinand_manufacturer, > >>> µn_spinand_manufacturer, > >>> + &toshiba_spinand_manufacturer, > >>> &winbond_spinand_manufacturer, > >>> }; > >>> > >>> diff --git a/drivers/mtd/nand/spi/toshiba.c b/drivers/mtd/nand/spi/toshiba.c > >>> new file mode 100644 > >>> index 0000000..294bcf6 > >>> --- /dev/null > >>> +++ b/drivers/mtd/nand/spi/toshiba.c > >>> @@ -0,0 +1,136 @@ > >>> +// SPDX-License-Identifier: GPL-2.0 > >>> +/* > >>> + * Copyright (c) 2018 exceet electronics GmbH > >>> + * Copyright (c) 2018 Kontron Electronics GmbH > >>> + * > >>> + * Author: Frieder Schrempf <frieder.schrempf@kontron.de> > >>> + */ > >>> + > >>> +#include <linux/device.h> > >>> +#include <linux/kernel.h> > >>> +#include <linux/mtd/spinand.h> > >>> + > >>> +#define SPINAND_MFR_TOSHIBA 0x98 > >>> + > >>> +static SPINAND_OP_VARIANTS(read_cache_variants, > >>> + SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0), > >>> + SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0), > >>> + SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0), > >>> + SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0)); > >>> + > >>> +static SPINAND_OP_VARIANTS(write_cache_variants, > >>> + SPINAND_PROG_LOAD(true, 0, NULL, 0)); > >>> + > >>> +static SPINAND_OP_VARIANTS(update_cache_variants, > >>> + SPINAND_PROG_LOAD(false, 0, NULL, 0)); > >>> + > >>> +static int tc58cvg2s0h_ooblayout_ecc(struct mtd_info *mtd, int section, > >>> + struct mtd_oob_region *region) > >>> +{ > >>> + if (section > 7) > >>> + return -ERANGE; > >>> + > >>> + region->offset = 128 + 16 * section; > >>> + region->length = 16; > > > > Here you expose the ECC bits has 8 sections of 16 bytes. > > But regarding the datasheet this should not be accessed page 32. > > "The ECC parity code generated by internal ECC is stored in column > > addresses 4224-4351 and the users cannot access to these specific > > addresses when internal ECC is enabled." > > This is just to let the other layers know, where the bytes used for ECC > are. As long as the driver uses the on-chip ECC it won't write to this > area. So this is correct unless I misunderstood this concept. All the > other supported SPI NAND chips use the same approach. Ok for not write, but are you sure that if we try to read them the SPI will respond something logic and not some garbage ? When I read the datasheet it looks like that the read cmd will stop or send some random value when trying to read these columns. > > > > >>> + > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static int tc58cvg2s0h_ooblayout_free(struct mtd_info *mtd, int section, > >>> + struct mtd_oob_region *region) > >>> +{ > >>> + if (section > 7) > >>> + return -ERANGE; > >>> + > >>> + region->offset = 2 + 16 * section; > >>> + region->length = 14; > > > > This reserved 2 bytes for BBM for each section. > > But maybe we could declare this as 1 section of 128bytes: > > > > if (section) > > return -ERANGE; > > > > region->offset = 2; > > region->length = 126; > > The datasheet (p. 32) describes that the OOB data of a page is divided > into 8 sections. So why should we not reflect this in the software? > Probably it would be sufficient to reserve two bytes for the bad block > marker at the beginning, instead of two bytes in each section. But I'm > not sure about this and it doesn't really hurt. If the OOB are used one day (I think it's not the case actually), It will be more efficient to do only one request. Regards, Clement > > > >>> + > >>> > >>> + return 0; > >>> +} > >>> + > >>> +static const struct mtd_ooblayout_ops tc58cvg2s0h_ooblayout = { > >>> + .ecc = tc58cvg2s0h_ooblayout_ecc, > >>> + .free = tc58cvg2s0h_ooblayout_free, > >>> +}; > >>> + > >>> +static int tc58cvg2s0h_ecc_get_status(struct spinand_device *spinand, > >>> + u8 status) > >>> +{ > >>> + struct nand_device *nand = spinand_to_nand(spinand); > >>> + u8 mbf = 0; > >>> + struct spi_mem_op op = SPINAND_GET_FEATURE_OP(0x30, &mbf); > >>> + > >>> + switch (status & STATUS_ECC_MASK) { > >>> + case STATUS_ECC_NO_BITFLIPS: > >>> + return 0; > >>> + > >>> + case STATUS_ECC_UNCOR_ERROR: > >>> + return -EBADMSG; > >>> + > >>> + case STATUS_ECC_HAS_BITFLIPS: > >>> + /* > >>> + * Let's try to retrieve the real maximum number of bitflips > >>> + * in order to avoid forcing the wear-leveling layer to move > >>> + * data around if it's not necessary. > >>> + */ > >>> + if (spi_mem_exec_op(spinand->spimem, &op)) > >>> + return nand->eccreq.strength; > >>> + > >>> + mbf >>= 4; > >>> + > >>> + if (WARN_ON(mbf > nand->eccreq.strength || !mbf)) > >>> + return nand->eccreq.strength; > >>> + > >>> + return mbf; > >>> + > > > > These take take care of 0, 0x1 and 0x2 but not 0x3 which is valid > > Hm, you're right. We don't care if it is 0x1 or 0x3 (both mean that > bitflips were corrected), but we currently only check for 0x1. > > I will send a fix for this tomorrow. > > Thanks, > Frieder > > > page 26 of the datasheet : > > > > ECC status bits indicate the status of internal ECC operation > > 00b: No bit flips were detected in previous page read. > > 01b: Bit flips were detected and corrected. Bit flip count did not > > exceed the bit flip detection threshold. The threshold is set by bits > > [7:4] in address 10h in the feature table. > > 10b: Multiple bit flips were detected and not corrected. > > 11b: Bit flips were detected and corrected. Bit flip count exceeded > > the bit flip detection threshold. The threshold is set by bits [7:4] > > in address 10h in the feature table > > > > Regards, > > Clement > > > >>> + default: > >>> + break; > >>> + } > >>> + > >>> + return -EINVAL; > >>> +} > >>> + > >>> +static const struct spinand_info toshiba_spinand_table[] = { > >>> + SPINAND_INFO("TC58CVG2S0H", 0xCD, > >>> + NAND_MEMORG(1, 4096, 256, 64, 2048, 1, 1, 1), > >>> + NAND_ECCREQ(8, 512), > >>> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants, > >>> + &write_cache_variants, > >>> + &update_cache_variants), > >>> + SPINAND_HAS_QE_BIT, > >>> + SPINAND_ECCINFO(&tc58cvg2s0h_ooblayout, > >>> + tc58cvg2s0h_ecc_get_status)), > >>> +}; > >>> + > >>> +static int toshiba_spinand_detect(struct spinand_device *spinand) > >>> +{ > >>> + u8 *id = spinand->id.data; > >>> + int ret; > >>> + > >>> + /* > >>> + * Toshiba SPI NAND read ID needs a dummy byte, > >>> + * so the first byte in id is garbage. > >>> + */ > >>> + if (id[1] != SPINAND_MFR_TOSHIBA) > >>> + return 0; > >>> + > >>> + ret = spinand_match_and_init(spinand, toshiba_spinand_table, > >>> + ARRAY_SIZE(toshiba_spinand_table), > >>> + id[2]); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + return 1; > >>> +} > >>> + > >>> +static const struct spinand_manufacturer_ops toshiba_spinand_manuf_ops = { > >>> + .detect = toshiba_spinand_detect, > >>> +}; > >>> + > >>> +const struct spinand_manufacturer toshiba_spinand_manufacturer = { > >>> + .id = SPINAND_MFR_TOSHIBA, > >>> + .name = "Toshiba", > >>> + .ops = &toshiba_spinand_manuf_ops, > >>> +}; > >>> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h > >>> index 088ff96..816c4b0 100644 > >>> --- a/include/linux/mtd/spinand.h > >>> +++ b/include/linux/mtd/spinand.h > >>> @@ -196,6 +196,7 @@ struct spinand_manufacturer { > >>> /* SPI NAND manufacturers */ > >>> extern const struct spinand_manufacturer macronix_spinand_manufacturer; > >>> extern const struct spinand_manufacturer micron_spinand_manufacturer; > >>> +extern const struct spinand_manufacturer toshiba_spinand_manufacturer; > >>> extern const struct spinand_manufacturer winbond_spinand_manufacturer; > >>> > >>> /** > >>>
Hi Clément, On 27.11.18 18:02, Clément Péron wrote: > Hi Frieder, > > On Tue, 27 Nov 2018 at 17:42, Schrempf Frieder > <frieder.schrempf@kontron.de> wrote: >> >> Hi Clément, >> >> On 27.11.18 16:18, Clément Péron wrote: >>> Hi Frieder, >>> >>> On Tue, 27 Nov 2018 at 16:08, Schrempf Frieder >>> <frieder.schrempf@kontron.de> wrote: >>>> >>>> +Clément Péron >>>> >>>> Hi Clément, >>>> >>>> FYI, this has already been merged to nand/next. >>> Just want to point the difference that I made when I try to introduce my driver. >>> The datasheet I used is this one : >>> https://media.digikey.com/pdf/Data%20Sheets/Toshiba%20PDFs/TC58CVG2S0HxAIx_Rev1.1_2016-11-08.pdf >>> >>>> >>>> Regards, >>>> Frieder >>>> >>>> On 08.11.18 09:29, Frieder Schrempf wrote: >>>>> Add minimal support for the Toshiba TC58CVG2S0H SPI NAND chip. >>>>> >>>>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de> >>>>> --- >>>>> drivers/mtd/nand/spi/Makefile | 2 +- >>>>> drivers/mtd/nand/spi/core.c | 1 + >>>>> drivers/mtd/nand/spi/toshiba.c | 136 ++++++++++++++++++++++++++++++++++++ >>>>> include/linux/mtd/spinand.h | 1 + >>>>> 4 files changed, 139 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile >>>>> index b74e074..be5f735 100644 >>>>> --- a/drivers/mtd/nand/spi/Makefile >>>>> +++ b/drivers/mtd/nand/spi/Makefile >>>>> @@ -1,3 +1,3 @@ >>>>> # SPDX-License-Identifier: GPL-2.0 >>>>> -spinand-objs := core.o macronix.o micron.o winbond.o >>>>> +spinand-objs := core.o macronix.o micron.o toshiba.o winbond.o >>>>> obj-$(CONFIG_MTD_SPI_NAND) += spinand.o >>>>> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c >>>>> index 30f8364..87bdf2a 100644 >>>>> --- a/drivers/mtd/nand/spi/core.c >>>>> +++ b/drivers/mtd/nand/spi/core.c >>>>> @@ -766,6 +766,7 @@ static const struct nand_ops spinand_ops = { >>>>> static const struct spinand_manufacturer *spinand_manufacturers[]= { >>>>> ¯onix_spinand_manufacturer, >>>>> µn_spinand_manufacturer, >>>>> + &toshiba_spinand_manufacturer, >>>>> &winbond_spinand_manufacturer, >>>>> }; >>>>> >>>>> diff --git a/drivers/mtd/nand/spi/toshiba.c b/drivers/mtd/nand/spi/toshiba.c >>>>> new file mode 100644 >>>>> index 0000000..294bcf6 >>>>> --- /dev/null >>>>> +++ b/drivers/mtd/nand/spi/toshiba.c >>>>> @@ -0,0 +1,136 @@ >>>>> +// SPDX-License-Identifier: GPL-2.0 >>>>> +/* >>>>> + * Copyright (c) 2018 exceet electronics GmbH >>>>> + * Copyright (c) 2018 Kontron Electronics GmbH >>>>> + * >>>>> + * Author: Frieder Schrempf <frieder.schrempf@kontron.de> >>>>> + */ >>>>> + >>>>> +#include <linux/device.h> >>>>> +#include <linux/kernel.h> >>>>> +#include <linux/mtd/spinand.h> >>>>> + >>>>> +#define SPINAND_MFR_TOSHIBA 0x98 >>>>> + >>>>> +static SPINAND_OP_VARIANTS(read_cache_variants, >>>>> + SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0), >>>>> + SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0), >>>>> + SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0), >>>>> + SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0)); >>>>> + >>>>> +static SPINAND_OP_VARIANTS(write_cache_variants, >>>>> + SPINAND_PROG_LOAD(true, 0, NULL, 0)); >>>>> + >>>>> +static SPINAND_OP_VARIANTS(update_cache_variants, >>>>> + SPINAND_PROG_LOAD(false, 0, NULL, 0)); >>>>> + >>>>> +static int tc58cvg2s0h_ooblayout_ecc(struct mtd_info *mtd, int section, >>>>> + struct mtd_oob_region *region) >>>>> +{ >>>>> + if (section > 7) >>>>> + return -ERANGE; >>>>> + >>>>> + region->offset = 128 + 16 * section; >>>>> + region->length = 16; >>> >>> Here you expose the ECC bits has 8 sections of 16 bytes. >>> But regarding the datasheet this should not be accessed page 32. >>> "The ECC parity code generated by internal ECC is stored in column >>> addresses 4224-4351 and the users cannot access to these specific >>> addresses when internal ECC is enabled." >> >> This is just to let the other layers know, where the bytes used for ECC >> are. As long as the driver uses the on-chip ECC it won't write to this >> area. So this is correct unless I misunderstood this concept. All the >> other supported SPI NAND chips use the same approach. > > Ok for not write, but are you sure that if we try to read them the SPI > will respond something logic and not some garbage ? > When I read the datasheet it looks like that the read cmd will stop or > send some random value when trying to read these columns. Maybe you are right. The datasheet says we shouldn't *access* ECC bytes, which includes read and write. I guess it won't hurt to block access to the ECC bytes as they are currently of no use anyway and I just saw that the Macronix driver is doing the same. >>>>> + >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int tc58cvg2s0h_ooblayout_free(struct mtd_info *mtd, int section, >>>>> + struct mtd_oob_region *region) >>>>> +{ >>>>> + if (section > 7) >>>>> + return -ERANGE; >>>>> + >>>>> + region->offset = 2 + 16 * section; >>>>> + region->length = 14; >>> >>> This reserved 2 bytes for BBM for each section. >>> But maybe we could declare this as 1 section of 128bytes: >>> >>> if (section) >>> return -ERANGE; >>> >>> region->offset = 2; >>> region->length = 126; >> >> The datasheet (p. 32) describes that the OOB data of a page is divided >> into 8 sections. So why should we not reflect this in the software? >> Probably it would be sufficient to reserve two bytes for the bad block >> marker at the beginning, instead of two bytes in each section. But I'm >> not sure about this and it doesn't really hurt. > > If the OOB are used one day (I think it's not the case actually), It > will be more efficient to do only one request. Ok, and even more so we already have a default OOB layout in the core driver, that could be reused for this case and also in other SPI NAND drivers. So we could actually get rid of a few lines of code. Thanks, Frieder >>>>> + >>>>> >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static const struct mtd_ooblayout_ops tc58cvg2s0h_ooblayout = { >>>>> + .ecc = tc58cvg2s0h_ooblayout_ecc, >>>>> + .free = tc58cvg2s0h_ooblayout_free, >>>>> +}; >>>>> + >>>>> +static int tc58cvg2s0h_ecc_get_status(struct spinand_device *spinand, >>>>> + u8 status) >>>>> +{ >>>>> + struct nand_device *nand = spinand_to_nand(spinand); >>>>> + u8 mbf = 0; >>>>> + struct spi_mem_op op = SPINAND_GET_FEATURE_OP(0x30, &mbf); >>>>> + >>>>> + switch (status & STATUS_ECC_MASK) { >>>>> + case STATUS_ECC_NO_BITFLIPS: >>>>> + return 0; >>>>> + >>>>> + case STATUS_ECC_UNCOR_ERROR: >>>>> + return -EBADMSG; >>>>> + >>>>> + case STATUS_ECC_HAS_BITFLIPS: >>>>> + /* >>>>> + * Let's try to retrieve the real maximum number of bitflips >>>>> + * in order to avoid forcing the wear-leveling layer tomove >>>>> + * data around if it's not necessary. >>>>> + */ >>>>> + if (spi_mem_exec_op(spinand->spimem, &op)) >>>>> + return nand->eccreq.strength; >>>>> + >>>>> + mbf >>= 4; >>>>> + >>>>> + if (WARN_ON(mbf > nand->eccreq.strength || !mbf)) >>>>> + return nand->eccreq.strength; >>>>> + >>>>> + return mbf; >>>>> + >>> >>> These take take care of 0, 0x1 and 0x2 but not 0x3 which is valid >> >> Hm, you're right. We don't care if it is 0x1 or 0x3 (both mean that >> bitflips were corrected), but we currently only check for 0x1. >> >> I will send a fix for this tomorrow. >> >> Thanks, >> Frieder >> >>> page 26 of the datasheet : >>> >>> ECC status bits indicate the status of internal ECC operation >>> 00b: No bit flips were detected in previous page read. >>> 01b: Bit flips were detected and corrected. Bit flip count did not >>> exceed the bit flip detection threshold. The threshold is set by bits >>> [7:4] in address 10h in the feature table. >>> 10b: Multiple bit flips were detected and not corrected. >>> 11b: Bit flips were detected and corrected. Bit flip count exceeded >>> the bit flip detection threshold. The threshold is set by bits [7:4] >>> in address 10h in the feature table >>> >>> Regards, >>> Clement >>> >>>>> + default: >>>>> + break; >>>>> + } >>>>> + >>>>> + return -EINVAL; >>>>> +} >>>>> + >>>>> +static const struct spinand_info toshiba_spinand_table[] = { >>>>> + SPINAND_INFO("TC58CVG2S0H", 0xCD, >>>>> + NAND_MEMORG(1, 4096, 256, 64, 2048, 1, 1, 1), >>>>> + NAND_ECCREQ(8, 512), >>>>> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants, >>>>> + &write_cache_variants, >>>>> + &update_cache_variants), >>>>> + SPINAND_HAS_QE_BIT, >>>>> + SPINAND_ECCINFO(&tc58cvg2s0h_ooblayout, >>>>> + tc58cvg2s0h_ecc_get_status)), >>>>> +}; >>>>> + >>>>> +static int toshiba_spinand_detect(struct spinand_device *spinand) >>>>> +{ >>>>> + u8 *id = spinand->id.data; >>>>> + int ret; >>>>> + >>>>> + /* >>>>> + * Toshiba SPI NAND read ID needs a dummy byte, >>>>> + * so the first byte in id is garbage. >>>>> + */ >>>>> + if (id[1] != SPINAND_MFR_TOSHIBA) >>>>> + return 0; >>>>> + >>>>> + ret = spinand_match_and_init(spinand, toshiba_spinand_table, >>>>> + ARRAY_SIZE(toshiba_spinand_table), >>>>> + id[2]); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + return 1; >>>>> +} >>>>> + >>>>> +static const struct spinand_manufacturer_ops toshiba_spinand_manuf_ops = { >>>>> + .detect = toshiba_spinand_detect, >>>>> +}; >>>>> + >>>>> +const struct spinand_manufacturer toshiba_spinand_manufacturer = { >>>>> + .id = SPINAND_MFR_TOSHIBA, >>>>> + .name = "Toshiba", >>>>> + .ops = &toshiba_spinand_manuf_ops, >>>>> +}; >>>>> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h >>>>> index 088ff96..816c4b0 100644 >>>>> --- a/include/linux/mtd/spinand.h >>>>> +++ b/include/linux/mtd/spinand.h >>>>> @@ -196,6 +196,7 @@ struct spinand_manufacturer { >>>>> /* SPI NAND manufacturers */ >>>>> extern const struct spinand_manufacturer macronix_spinand_manufacturer; >>>>> extern const struct spinand_manufacturer micron_spinand_manufacturer; >>>>> +extern const struct spinand_manufacturer toshiba_spinand_manufacturer; >>>>> extern const struct spinand_manufacturer winbond_spinand_manufacturer; >>>>> >>>>> /** >>>>>
Hi Miquèl, On 18.11.18 21:47, Miquel Raynal wrote: > Hi Schrempf, > > Schrempf Frieder <frieder.schrempf@kontron.De> wrote on Thu, 8 Nov 2018 > 08:32:11 +0000: > >> Add minimal support for the Toshiba TC58CVG2S0H SPI NAND chip. >> >> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de> >> --- > > With "mtd: spinand:" as prefix, applied to nand/next. Clément suggested some fixes/improvements for this patch. You can find them in this patch: https://patchwork.ozlabs.org/patch/1004501/. When these changes are approved, you can decide if you apply this on top of the initial patch or if you can/want to squash both patches. Thanks, Frieder
On Tue, 27 Nov 2018 16:41:56 +0000 Schrempf Frieder <frieder.schrempf@kontron.de> wrote: > >>> +static int tc58cvg2s0h_ooblayout_ecc(struct mtd_info *mtd, int section, > >>> + struct mtd_oob_region *region) > >>> +{ > >>> + if (section > 7) > >>> + return -ERANGE; > >>> + > >>> + region->offset = 128 + 16 * section; > >>> + region->length = 16; > > > > Here you expose the ECC bits has 8 sections of 16 bytes. > > But regarding the datasheet this should not be accessed page 32. > > "The ECC parity code generated by internal ECC is stored in column > > addresses 4224-4351 and the users cannot access to these specific > > addresses when internal ECC is enabled." 'when internal ECC is enabled' means that those bytes can be accessed when it's disabled. We should keep exposing the ECC byte sections. Note that even if ECC sections are not exposed, the core will read those bytes. They're probably filled with garbage in this case, but we don't care since we won't use them. > > This is just to let the other layers know, where the bytes used for ECC > are. As long as the driver uses the on-chip ECC it won't write to this > area. So this is correct unless I misunderstood this concept. All the > other supported SPI NAND chips use the same approach. Yes, and that's the right thing to do. We want to know where the ECC bytes are, especially when doing raw accesses. > > > > >>> + > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static int tc58cvg2s0h_ooblayout_free(struct mtd_info *mtd, int section, > >>> + struct mtd_oob_region *region) > >>> +{ > >>> + if (section > 7) > >>> + return -ERANGE; > >>> + > >>> + region->offset = 2 + 16 * section; > >>> + region->length = 14; > > > > This reserved 2 bytes for BBM for each section. > > But maybe we could declare this as 1 section of 128bytes: > > > > if (section) > > return -ERANGE; > > > > region->offset = 2; > > region->length = 126; I agree with this suggestion: if the free bytes are contiguous, it's better to expose them as a single section.
Hi Boris, On 28.11.18 13:53, Boris Brezillon wrote: > On Tue, 27 Nov 2018 16:41:56 +0000 > Schrempf Frieder <frieder.schrempf@kontron.de> wrote: > >>>>> +static int tc58cvg2s0h_ooblayout_ecc(struct mtd_info *mtd, int section, >>>>> + struct mtd_oob_region *region) >>>>> +{ >>>>> + if (section > 7) >>>>> + return -ERANGE; >>>>> + >>>>> + region->offset = 128 + 16 * section; >>>>> + region->length = 16; >>> >>> Here you expose the ECC bits has 8 sections of 16 bytes. >>> But regarding the datasheet this should not be accessed page 32. >>> "The ECC parity code generated by internal ECC is stored in column >>> addresses 4224-4351 and the users cannot access to these specific >>> addresses when internal ECC is enabled." > > 'when internal ECC is enabled' means that those bytes can be accessed > when it's disabled. We should keep exposing the ECC byte sections. Note > that even if ECC sections are not exposed, the core will read those > bytes. They're probably filled with garbage in this case, but we don't > care since we won't use them. > >> >> This is just to let the other layers know, where the bytes used for ECC >> are. As long as the driver uses the on-chip ECC it won't write to this >> area. So this is correct unless I misunderstood this concept. All the >> other supported SPI NAND chips use the same approach. > > Yes, and that's the right thing to do. We want to know where the ECC > bytes are, especially when doing raw accesses. Thank you for clarifying this. I will send a v2 of my patch and revert this change. Thanks, Frieder > >> >>> >>>>> + >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int tc58cvg2s0h_ooblayout_free(struct mtd_info *mtd, int section, >>>>> + struct mtd_oob_region *region) >>>>> +{ >>>>> + if (section > 7) >>>>> + return -ERANGE; >>>>> + >>>>> + region->offset = 2 + 16 * section; >>>>> + region->length = 14; >>> >>> This reserved 2 bytes for BBM for each section. >>> But maybe we could declare this as 1 section of 128bytes: >>> >>> if (section) >>> return -ERANGE; >>> >>> region->offset = 2; >>> region->length = 126; > > I agree with this suggestion: if the free bytes are contiguous, it's > better to expose them as a single section. >
diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile index b74e074..be5f735 100644 --- a/drivers/mtd/nand/spi/Makefile +++ b/drivers/mtd/nand/spi/Makefile @@ -1,3 +1,3 @@ # SPDX-License-Identifier: GPL-2.0 -spinand-objs := core.o macronix.o micron.o winbond.o +spinand-objs := core.o macronix.o micron.o toshiba.o winbond.o obj-$(CONFIG_MTD_SPI_NAND) += spinand.o diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c index 30f8364..87bdf2a 100644 --- a/drivers/mtd/nand/spi/core.c +++ b/drivers/mtd/nand/spi/core.c @@ -766,6 +766,7 @@ static const struct nand_ops spinand_ops = { static const struct spinand_manufacturer *spinand_manufacturers[] = { ¯onix_spinand_manufacturer, µn_spinand_manufacturer, + &toshiba_spinand_manufacturer, &winbond_spinand_manufacturer, }; diff --git a/drivers/mtd/nand/spi/toshiba.c b/drivers/mtd/nand/spi/toshiba.c new file mode 100644 index 0000000..294bcf6 --- /dev/null +++ b/drivers/mtd/nand/spi/toshiba.c @@ -0,0 +1,136 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2018 exceet electronics GmbH + * Copyright (c) 2018 Kontron Electronics GmbH + * + * Author: Frieder Schrempf <frieder.schrempf@kontron.de> + */ + +#include <linux/device.h> +#include <linux/kernel.h> +#include <linux/mtd/spinand.h> + +#define SPINAND_MFR_TOSHIBA 0x98 + +static SPINAND_OP_VARIANTS(read_cache_variants, + SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0), + SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0), + SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0), + SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0)); + +static SPINAND_OP_VARIANTS(write_cache_variants, + SPINAND_PROG_LOAD(true, 0, NULL, 0)); + +static SPINAND_OP_VARIANTS(update_cache_variants, + SPINAND_PROG_LOAD(false, 0, NULL, 0)); + +static int tc58cvg2s0h_ooblayout_ecc(struct mtd_info *mtd, int section, + struct mtd_oob_region *region) +{ + if (section > 7) + return -ERANGE; + + region->offset = 128 + 16 * section; + region->length = 16; + + + return 0; +} + +static int tc58cvg2s0h_ooblayout_free(struct mtd_info *mtd, int section, + struct mtd_oob_region *region) +{ + if (section > 7) + return -ERANGE; + + region->offset = 2 + 16 * section; + region->length = 14; + + + return 0; +} + +static const struct mtd_ooblayout_ops tc58cvg2s0h_ooblayout = { + .ecc = tc58cvg2s0h_ooblayout_ecc, + .free = tc58cvg2s0h_ooblayout_free, +}; + +static int tc58cvg2s0h_ecc_get_status(struct spinand_device *spinand, + u8 status) +{ + struct nand_device *nand = spinand_to_nand(spinand); + u8 mbf = 0; + struct spi_mem_op op = SPINAND_GET_FEATURE_OP(0x30, &mbf); + + switch (status & STATUS_ECC_MASK) { + case STATUS_ECC_NO_BITFLIPS: + return 0; + + case STATUS_ECC_UNCOR_ERROR: + return -EBADMSG; + + case STATUS_ECC_HAS_BITFLIPS: + /* + * Let's try to retrieve the real maximum number of bitflips + * in order to avoid forcing the wear-leveling layer to move + * data around if it's not necessary. + */ + if (spi_mem_exec_op(spinand->spimem, &op)) + return nand->eccreq.strength; + + mbf >>= 4; + + if (WARN_ON(mbf > nand->eccreq.strength || !mbf)) + return nand->eccreq.strength; + + return mbf; + + default: + break; + } + + return -EINVAL; +} + +static const struct spinand_info toshiba_spinand_table[] = { + SPINAND_INFO("TC58CVG2S0H", 0xCD, + NAND_MEMORG(1, 4096, 256, 64, 2048, 1, 1, 1), + NAND_ECCREQ(8, 512), + SPINAND_INFO_OP_VARIANTS(&read_cache_variants, + &write_cache_variants, + &update_cache_variants), + SPINAND_HAS_QE_BIT, + SPINAND_ECCINFO(&tc58cvg2s0h_ooblayout, + tc58cvg2s0h_ecc_get_status)), +}; + +static int toshiba_spinand_detect(struct spinand_device *spinand) +{ + u8 *id = spinand->id.data; + int ret; + + /* + * Toshiba SPI NAND read ID needs a dummy byte, + * so the first byte in id is garbage. + */ + if (id[1] != SPINAND_MFR_TOSHIBA) + return 0; + + ret = spinand_match_and_init(spinand, toshiba_spinand_table, + ARRAY_SIZE(toshiba_spinand_table), + id[2]); + if (ret) + return ret; + + return 1; +} + +static const struct spinand_manufacturer_ops toshiba_spinand_manuf_ops = { + .detect = toshiba_spinand_detect, +}; + +const struct spinand_manufacturer toshiba_spinand_manufacturer = { + .id = SPINAND_MFR_TOSHIBA, + .name = "Toshiba", + .ops = &toshiba_spinand_manuf_ops, +}; diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h index 088ff96..816c4b0 100644 --- a/include/linux/mtd/spinand.h +++ b/include/linux/mtd/spinand.h @@ -196,6 +196,7 @@ struct spinand_manufacturer { /* SPI NAND manufacturers */ extern const struct spinand_manufacturer macronix_spinand_manufacturer; extern const struct spinand_manufacturer micron_spinand_manufacturer; +extern const struct spinand_manufacturer toshiba_spinand_manufacturer; extern const struct spinand_manufacturer winbond_spinand_manufacturer; /**
Add minimal support for the Toshiba TC58CVG2S0H SPI NAND chip. Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de> --- drivers/mtd/nand/spi/Makefile | 2 +- drivers/mtd/nand/spi/core.c | 1 + drivers/mtd/nand/spi/toshiba.c | 136 ++++++++++++++++++++++++++++++++++++ include/linux/mtd/spinand.h | 1 + 4 files changed, 139 insertions(+), 1 deletion(-)