Re: [PATCH] mtd: nand: spi: Add initial support for Toshiba TC58CVG2S0H

From: ClÃment PÃron
Date: Tue Nov 27 2018 - 12:02:33 EST

Hi Frieder,

On Tue, 27 Nov 2018 at 17:42, Schrempf Frieder
<frieder.schrempf@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> >>> ---
> >>> 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[] = {
> >>> &macronix_spinand_manufacturer,
> >>> &micron_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@xxxxxxxxxx>
> >>> + */
> >>> +
> >>> +#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;
> >>>
> >>> /**
> >>>