[V2] mtd: spinand: Add support for Etron EM73D044VCx

[V2] mtd: spinand: Add support for Etron EM73D044VCx

[Bert Vermeulen]

This adds a new vendor Etron, and support for a 2Gb chip.

The datasheet is available at
https://www.etron.com/cn/products/EM73%5B8%5DC%5BD_E_F%5DVC%20SPI%20NAND%20Flash_Promotion_Rev%201_00A.pdf

Signed-off-by: Bert Vermeulen <bert@biot.com>
---
v2: Fixed a small style issue, and now sending to linux-mtd@ as well.

 drivers/mtd/nand/spi/Makefile |  2 +-
 drivers/mtd/nand/spi/core.c   |  1 +
 drivers/mtd/nand/spi/etron.c  | 98 +++++++++++++++++++++++++++++++++++
 include/linux/mtd/spinand.h   |  1 +
 4 files changed, 101 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mtd/nand/spi/etron.c

diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile
index 9662b9c1d5a9..cc3c4e046ea9 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 gigadevice.o macronix.o micron.o paragon.o toshiba.o winbond.o
+spinand-objs := core.o etron.o gigadevice.o macronix.o micron.o paragon.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 446ba8d43fbc..2a806ffa7310 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -895,6 +895,7 @@ static const struct nand_ops spinand_ops = {
 
 static const struct spinand_manufacturer *spinand_manufacturers[] = {
 	&gigadevice_spinand_manufacturer,
+	&etron_spinand_manufacturer,
 	&macronix_spinand_manufacturer,
 	&micron_spinand_manufacturer,
 	&paragon_spinand_manufacturer,
diff --git a/drivers/mtd/nand/spi/etron.c b/drivers/mtd/nand/spi/etron.c
new file mode 100644
index 000000000000..653092be5938
--- /dev/null
+++ b/drivers/mtd/nand/spi/etron.c
@@ -0,0 +1,98 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/mtd/spinand.h>
+
+#define SPINAND_MFR_ETRON			0xd5
+
+
+static SPINAND_OP_VARIANTS(read_cache_variants,
+		SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 1, NULL, 0),
+		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
+		SPINAND_PAGE_READ_FROM_CACHE_DUALIO_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_X4(true, 0, NULL, 0),
+		SPINAND_PROG_LOAD(true, 0, NULL, 0));
+
+static SPINAND_OP_VARIANTS(update_cache_variants,
+		SPINAND_PROG_LOAD_X4(false, 0, NULL, 0),
+		SPINAND_PROG_LOAD(false, 0, NULL, 0));
+
+static int etron_ooblayout_ecc(struct mtd_info *mtd, int section,
+					struct mtd_oob_region *oobregion)
+{
+	if (section)
+		return -ERANGE;
+
+	oobregion->offset = 72;
+	oobregion->length = 56;
+
+	return 0;
+}
+
+static int etron_ooblayout_free(struct mtd_info *mtd, int section,
+			   struct mtd_oob_region *oobregion)
+{
+	if (section)
+		return -ERANGE;
+
+	oobregion->offset = 1;
+	oobregion->length = 71;
+
+	return 0;
+}
+
+static int etron_ecc_get_status(struct spinand_device *spinand, u8 status)
+{
+	switch (status & STATUS_ECC_MASK) {
+	case STATUS_ECC_NO_BITFLIPS:
+		return 0;
+
+	case STATUS_ECC_HAS_BITFLIPS:
+		/* Between 1-7 bitflips were corrected */
+		return 7;
+
+	case STATUS_ECC_MASK:
+		/* Maximum bitflips were corrected */
+		return 8;
+
+	case STATUS_ECC_UNCOR_ERROR:
+		return -EBADMSG;
+	}
+
+	return -EINVAL;
+}
+
+static const struct mtd_ooblayout_ops etron_ooblayout = {
+	.ecc = etron_ooblayout_ecc,
+	.free = etron_ooblayout_free,
+};
+
+static const struct spinand_info etron_spinand_table[] = {
+	SPINAND_INFO("EM73D044VCx",
+		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_ADDR, 0x1f),
+		     // bpc, pagesize, oobsize, pagesperblock, bperlun, maxbadplun, ppl, lpt, #t
+		     NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 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(&etron_ooblayout, etron_ecc_get_status)),
+};
+
+static const struct spinand_manufacturer_ops etron_spinand_manuf_ops = {
+};
+
+const struct spinand_manufacturer etron_spinand_manufacturer = {
+	.id = SPINAND_MFR_ETRON,
+	.name = "Etron",
+	.chips = etron_spinand_table,
+	.nchips = ARRAY_SIZE(etron_spinand_table),
+	.ops = &etron_spinand_manuf_ops,
+};
diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index 6988956b8492..d7c0a0439652 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -260,6 +260,7 @@ struct spinand_manufacturer {
 };
 
 /* SPI NAND manufacturers */
+extern const struct spinand_manufacturer etron_spinand_manufacturer;
 extern const struct spinand_manufacturer gigadevice_spinand_manufacturer;
 extern const struct spinand_manufacturer macronix_spinand_manufacturer;
 extern const struct spinand_manufacturer micron_spinand_manufacturer;
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [V2] mtd: spinand: Add support for Etron EM73D044VCx

[Miquel Raynal]

Hi Bert,

Bert Vermeulen <bert@biot.com> wrote on Thu, 29 Jul 2021 17:18:23 +0200:

> This adds a new vendor Etron, and support for a 2Gb chip.

Interesting name :)

> The datasheet is available at
> https://www.etron.com/cn/products/EM73%5B8%5DC%5BD_E_F%5DVC%20SPI%20NAND%20Flash_Promotion_Rev%201_00A.pdf
> 
> Signed-off-by: Bert Vermeulen <bert@biot.com>
> ---
> v2: Fixed a small style issue, and now sending to linux-mtd@ as well.
> 
>  drivers/mtd/nand/spi/Makefile |  2 +-
>  drivers/mtd/nand/spi/core.c   |  1 +
>  drivers/mtd/nand/spi/etron.c  | 98 +++++++++++++++++++++++++++++++++++
>  include/linux/mtd/spinand.h   |  1 +
>  4 files changed, 101 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/mtd/nand/spi/etron.c
> 
> diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile
> index 9662b9c1d5a9..cc3c4e046ea9 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 gigadevice.o macronix.o micron.o paragon.o toshiba.o winbond.o
> +spinand-objs := core.o etron.o gigadevice.o macronix.o micron.o paragon.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 446ba8d43fbc..2a806ffa7310 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -895,6 +895,7 @@ static const struct nand_ops spinand_ops = {
>  
>  static const struct spinand_manufacturer *spinand_manufacturers[] = {
>  	&gigadevice_spinand_manufacturer,
> +	&etron_spinand_manufacturer,

Should be placed above :)

>  	&macronix_spinand_manufacturer,
>  	&micron_spinand_manufacturer,
>  	&paragon_spinand_manufacturer,
> diff --git a/drivers/mtd/nand/spi/etron.c b/drivers/mtd/nand/spi/etron.c
> new file mode 100644
> index 000000000000..653092be5938
> --- /dev/null
> +++ b/drivers/mtd/nand/spi/etron.c
> @@ -0,0 +1,98 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/mtd/spinand.h>
> +
> +#define SPINAND_MFR_ETRON			0xd5
> +
> +

No double \n please

> +static SPINAND_OP_VARIANTS(read_cache_variants,
> +		SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 1, NULL, 0),
> +		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
> +		SPINAND_PAGE_READ_FROM_CACHE_DUALIO_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_X4(true, 0, NULL, 0),
> +		SPINAND_PROG_LOAD(true, 0, NULL, 0));
> +
> +static SPINAND_OP_VARIANTS(update_cache_variants,
> +		SPINAND_PROG_LOAD_X4(false, 0, NULL, 0),
> +		SPINAND_PROG_LOAD(false, 0, NULL, 0));
> +
> +static int etron_ooblayout_ecc(struct mtd_info *mtd, int section,
> +					struct mtd_oob_region *oobregion)
> +{
> +	if (section)
> +		return -ERANGE;
> +
> +	oobregion->offset = 72;
> +	oobregion->length = 56;

This looks highly dependent on a single chip, could you make these
values (at least the offset) dependent on the MTD device?

> +
> +	return 0;
> +}
> +
> +static int etron_ooblayout_free(struct mtd_info *mtd, int section,
> +			   struct mtd_oob_region *oobregion)
> +{
> +	if (section)
> +		return -ERANGE;
> +
> +	oobregion->offset = 1;
> +	oobregion->length = 71;

Same here.

> +
> +	return 0;
> +}
> +
> +static int etron_ecc_get_status(struct spinand_device *spinand, u8 status)
> +{
> +	switch (status & STATUS_ECC_MASK) {
> +	case STATUS_ECC_NO_BITFLIPS:
> +		return 0;
> +
> +	case STATUS_ECC_HAS_BITFLIPS:
> +		/* Between 1-7 bitflips were corrected */
> +		return 7;
> +
> +	case STATUS_ECC_MASK:
> +		/* Maximum bitflips were corrected */
> +		return 8;
> +
> +	case STATUS_ECC_UNCOR_ERROR:
> +		return -EBADMSG;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct mtd_ooblayout_ops etron_ooblayout = {
> +	.ecc = etron_ooblayout_ecc,
> +	.free = etron_ooblayout_free,
> +};

Could be above _get_status()

> +
> +static const struct spinand_info etron_spinand_table[] = {
> +	SPINAND_INFO("EM73D044VCx",
> +		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_ADDR, 0x1f),
> +		     // bpc, pagesize, oobsize, pagesperblock, bperlun, maxbadplun, ppl, lpt, #t

Please drop this comment

> +		     NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 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(&etron_ooblayout, etron_ecc_get_status)),
> +};
> +
> +static const struct spinand_manufacturer_ops etron_spinand_manuf_ops = {
> +};
> +
> +const struct spinand_manufacturer etron_spinand_manufacturer = {
> +	.id = SPINAND_MFR_ETRON,
> +	.name = "Etron",
> +	.chips = etron_spinand_table,
> +	.nchips = ARRAY_SIZE(etron_spinand_table),
> +	.ops = &etron_spinand_manuf_ops,
> +};
> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> index 6988956b8492..d7c0a0439652 100644
> --- a/include/linux/mtd/spinand.h
> +++ b/include/linux/mtd/spinand.h
> @@ -260,6 +260,7 @@ struct spinand_manufacturer {
>  };
>  
>  /* SPI NAND manufacturers */
> +extern const struct spinand_manufacturer etron_spinand_manufacturer;
>  extern const struct spinand_manufacturer gigadevice_spinand_manufacturer;
>  extern const struct spinand_manufacturer macronix_spinand_manufacturer;
>  extern const struct spinand_manufacturer micron_spinand_manufacturer;

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [V2] mtd: spinand: Add support for Etron EM73D044VCx

[Bert Vermeulen]

On 8/6/21 9:34 PM, Miquel Raynal wrote:

Hi Miquel,

Thanks for reviewing. I'll send in a v2 with fixes. However...

> Bert Vermeulen <bert@biot.com> wrote on Thu, 29 Jul 2021 17:18:23 +0200:
>> +static int etron_ooblayout_ecc(struct mtd_info *mtd, int section,
>> +					struct mtd_oob_region *oobregion)
>> +{
>> +	if (section)
>> +		return -ERANGE;
>> +
>> +	oobregion->offset = 72;
>> +	oobregion->length = 56;
> 
> This looks highly dependent on a single chip, could you make these
> values (at least the offset) dependent on the MTD device?
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int etron_ooblayout_free(struct mtd_info *mtd, int section,
>> +			   struct mtd_oob_region *oobregion)
>> +{
>> +	if (section)
>> +		return -ERANGE;
>> +
>> +	oobregion->offset = 1;
>> +	oobregion->length = 71;
> 
> Same here.

I was rather hoping for some guidance on the _ecc and _free
offset/length numbers. There's not much info around on what's best
practice here, and I haven't played with NAND chips enough to actually
test the in-kernel ECC stuff.

The _free offset of 1 is for the bad block byte, which I saw in another
NAND chip driver, but I don't remember where I got the other numbers
from, and really need somebody to double-check that.

thanks,


-- 
Bert Vermeulen
bert@biot.com

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [V2] mtd: spinand: Add support for Etron EM73D044VCx

[Miquel Raynal]

Hi Bert,

Bert Vermeulen <bert@biot.com> wrote on Sat, 7 Aug 2021 09:31:55 +0200:

> On 8/6/21 9:34 PM, Miquel Raynal wrote:
> 
> Hi Miquel,
> 
> Thanks for reviewing. I'll send in a v2 with fixes. However...
> 
> > Bert Vermeulen <bert@biot.com> wrote on Thu, 29 Jul 2021 17:18:23 +0200:  
> >> +static int etron_ooblayout_ecc(struct mtd_info *mtd, int section,
> >> +					struct mtd_oob_region *oobregion)
> >> +{
> >> +	if (section)
> >> +		return -ERANGE;
> >> +
> >> +	oobregion->offset = 72;
> >> +	oobregion->length = 56;  
> > 
> > This looks highly dependent on a single chip, could you make these
> > values (at least the offset) dependent on the MTD device?
> >   
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int etron_ooblayout_free(struct mtd_info *mtd, int section,
> >> +			   struct mtd_oob_region *oobregion)
> >> +{
> >> +	if (section)
> >> +		return -ERANGE;
> >> +
> >> +	oobregion->offset = 1;
> >> +	oobregion->length = 71;  
> > 
> > Same here.  
> 
> I was rather hoping for some guidance on the _ecc and _free
> offset/length numbers. There's not much info around on what's best
> practice here, and I haven't played with NAND chips enough to actually
> test the in-kernel ECC stuff.
> 
> The _free offset of 1 is for the bad block byte, which I saw in another
> NAND chip driver, but I don't remember where I got the other numbers
> from, and really need somebody to double-check that.

These values should be in the datasheet somehow.

Depending on the algorithm (ex: BCH) and the strength (ex: 4b/512B) you
will need a certain number of ECC bytes per chunk. Then you multiply
that number by the number of chunks and you get the total number of ECC
bytes. Assuming you know an offset from the start or the end of the OOB
section, you might write something like that:

	length = mtd->writesize / chunk_size * nb_ecc_bytes/chunk;
	offset = mtd->oobsize - length;

Also please reserve two bytes for OOB (even if so far we encountered
only 1B markers).

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/