mtd: spinand: Add support for Etron EM73D044VCx

PATCH Superseded Bert Vermeulen 2 participants 4 messages Jul 29, 2021 – Aug 7, 2021

Subsystems (2)

Memory technology devices (MTD) NAND flash

lore

Share & tools

lore

Bert Vermeulen

author

2021-07-29 15:20 UTC #0 lore raw

PATCH v2 1/1 mtd: spinand: Add support for Etron EM73D044VCx

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 <[email protected]>

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

drivers/mtd/nand/spi/Makefile +1 -1

index 9662b9c1d5a9..cc3c4e046ea9 100644
@@ -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

drivers/mtd/nand/spi/core.c +1 -0

index 446ba8d43fbc..2a806ffa7310 100644
@@ -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,

drivers/mtd/nand/spi/etron.c new file +98 -0

98 lines not on this page · raw message

include/linux/mtd/spinand.h +1 -0

index 6988956b8492..d7c0a0439652 100644
@@ -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;

Signature · 6 lines

2.25.1


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

Miquel Raynal

maintainer

2021-08-06 19:34 UTC #1 lore raw

Hi Bert,

Bert Vermeulen <[email protected]> wrote on Thu, 29 Jul 2021 17:18:23 +0200:

Quoted 1 line

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

Interesting name :)

Quoted 28 lines

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 <[email protected]>
---
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
drivers/mtd/nand/spi/Makefile +1 -1
index 9662b9c1d5a9..cc3c4e046ea9 100644
@@ -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
drivers/mtd/nand/spi/core.c +0 -0
index 446ba8d43fbc..2a806ffa7310 100644

drivers/mtd/nand/spi/core.c static const struct nand_ops spinand_ops = { · line 898

Quoted 4 lines

drivers/mtd/nand/spi/core.c +1 -0

@@ -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 :)

Quoted 9 lines

drivers/mtd/nand/spi/core.c +0 -0

 	&macronix_spinand_manufacturer,
 	&micron_spinand_manufacturer,
 	&paragon_spinand_manufacturer,

drivers/mtd/nand/spi/etron.c new file +0 -0

new file mode 100644
index 000000000000..653092be5938

drivers/mtd/nand/spi/etron.c line 9

Quoted 9 lines

drivers/mtd/nand/spi/etron.c new file +9 -0

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

Quoted 14 lines

drivers/mtd/nand/spi/etron.c new file +14 -0

+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),

drivers/mtd/nand/spi/etron.c line 33

Quoted 10 lines

drivers/mtd/nand/spi/etron.c new file +10 -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?

drivers/mtd/nand/spi/etron.c line 45

Quoted 12 lines

drivers/mtd/nand/spi/etron.c new file +12 -0

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

Quoted 19 lines

drivers/mtd/nand/spi/etron.c new file +19 -0

+
+	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:

drivers/mtd/nand/spi/etron.c line 74

Quoted 10 lines

drivers/mtd/nand/spi/etron.c new file +10 -0

+		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()

drivers/mtd/nand/spi/etron.c line 79

Quoted 5 lines

drivers/mtd/nand/spi/etron.c new file +5 -0

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

Quoted 24 lines

drivers/mtd/nand/spi/etron.c new file +19 -0

+		     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,
+};

include/linux/mtd/spinand.h +0 -0

index 6988956b8492..d7c0a0439652 100644

include/linux/mtd/spinand.h struct spinand_manufacturer { · line 266

Quoted 7 lines

include/linux/mtd/spinand.h +1 -0

@@ -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/

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,

Signature · 6 lines

Bert Vermeulen
[email protected]

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

Miquel Raynal

maintainer

2021-08-07 10:26 UTC #3 lore raw

Hi Bert,

Bert Vermeulen <[email protected]> wrote on Sat, 7 Aug 2021 09:31:55 +0200:

Quoted 34 lines · Miquel Raynal

Hi Miquel,
Thanks for reviewing. I'll send in a v2 with fixes. However...
Quoted 13 lines
Bert Vermeulen <[email protected]> wrote on Thu, 29 Jul 2021 17:18:23 +0200:
drivers/mtd/nand/spi/etron.c line 33
Quoted 8 lines
drivers/mtd/nand/spi/etron.c new file +8 -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?
Quoted 14 lines
drivers/mtd/nand/spi/etron.c line 45
Quoted 12 lines
drivers/mtd/nand/spi/etron.c new file +12 -0
+
+	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.

Quoted 8 lines

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/