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

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

[Schrempf Frieder]

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[] = {
 	&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@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;
 
 /**
-- 
2.7.4

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

[Boris Brezillon]

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[] = {
>  	&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@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;
>  
>  /**

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

[Miquel Raynal]

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

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

[Schrempf Frieder]

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

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

[Schrempf Frieder]

+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[] = {
>   	&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@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;
>   
>   /**
> 

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

[Clément Péron]

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[] = {
> >       &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@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;
> >
> >   /**
> >

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

[Schrempf Frieder]

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[] = {
>>>        &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@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;
>>>
>>>    /**
>>>

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

[Clément Péron]

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[] = {
> >>>        &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@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;
> >>>
> >>>    /**
> >>>

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

[Schrempf Frieder]

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[]= {
>>>>>         &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@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;
>>>>>
>>>>>     /**
>>>>>

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

[Boris Brezillon]

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.

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

[Schrempf Frieder]

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.
>