mtd: spinand: Add support for AllianceMemory AS5F34G04SND

PATCH Superseded Mario Kicherer 2 participants 8 messages Jan 17, 2023 – Jan 26, 2023

Subsystems (2)

Memory technology devices (MTD) NAND flash

lore

Share & tools

lore

Mario Kicherer

author

2023-01-17 16:55 UTC #0 lore raw

PATCH v2 1/1 mtd: spinand: Add support for AllianceMemory AS5F34G04SND

Add support for AllianceMemory AS5F34G04SND SPI NAND flash

Datasheet:
- https://www.alliancememory.com/wp-content/uploads/pdf/flash/AllianceMemory_SPI_NAND_Flash_July2020_Rev1.0.pdf

Signed-off-by Mario Kicherer <[email protected]>

Reviewed-by Dhruva Gole <[email protected]>

---
Changes since v1:
- added missing email recipients, sorry for the noise!

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

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

index b520fe634041..4ec973b8b6bf 100644
@@ -1,3 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0
-spinand-objs := core.o ato.o gigadevice.o macronix.o micron.o paragon.o toshiba.o winbond.o xtx.o
+spinand-objs := core.o alliancememory.o ato.o gigadevice.o macronix.o micron.o paragon.o toshiba.o winbond.o xtx.o
 obj-$(CONFIG_MTD_SPI_NAND) += spinand.o

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

119 lines not on this page · raw message

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

index dacd9c0e8b20..638391f77d8c 100644
@@ -937,6 +937,7 @@ static const struct nand_ops spinand_ops = {
 };
 
 static const struct spinand_manufacturer *spinand_manufacturers[] = {
+	&alliancememory_spinand_manufacturer,
 	&ato_spinand_manufacturer,
 	&gigadevice_spinand_manufacturer,
 	&macronix_spinand_manufacturer,

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

index 6d3392a7edc6..01be9f0f008a 100644
@@ -260,6 +260,7 @@ struct spinand_manufacturer {
 };
 
 /* SPI NAND manufacturers */
+extern const struct spinand_manufacturer alliancememory_spinand_manufacturer;
 extern const struct spinand_manufacturer ato_spinand_manufacturer;
 extern const struct spinand_manufacturer gigadevice_spinand_manufacturer;
 extern const struct spinand_manufacturer macronix_spinand_manufacturer;

Signature · 6 lines

2.34.1


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

Miquel Raynal

maintainer

2023-01-23 10:34 UTC #1 lore raw

Hi Mario,

[email protected] wrote on Tue, 17 Jan 2023 17:54:41 +0100:

Quoted 1 line

Add support for AllianceMemory AS5F34G04SND SPI NAND flash

Thanks for you patch!

Quoted 64 lines

Datasheet:
- https://www.alliancememory.com/wp-content/uploads/pdf/flash/AllianceMemory_SPI_NAND_Flash_July2020_Rev1.0.pdf

Signed-off-by Mario Kicherer <[email protected]>

Reviewed-by Dhruva Gole <[email protected]>

---
Changes since v1:
- added missing email recipients, sorry for the noise!

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

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

index b520fe634041..4ec973b8b6bf 100644
@@ -1,3 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0
-spinand-objs := core.o ato.o gigadevice.o macronix.o micron.o paragon.o toshiba.o winbond.o xtx.o
+spinand-objs := core.o alliancememory.o ato.o gigadevice.o macronix.o micron.o paragon.o toshiba.o winbond.o xtx.o
 obj-$(CONFIG_MTD_SPI_NAND) += spinand.o

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

new file mode 100644
index 000000000000..7f4b90befd27
@@ -0,0 +1,119 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Author: Mario Kicherer <[email protected]>
+ */
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/mtd/spinand.h>
+
+#define SPINAND_MFR_ALLIANCEMEMORY	0x52
+
+#define AM_STATUS_ECC_BITMASK		(3 << 4)
+
+#define AM_STATUS_ECC_NONE_DETECTED	(0 << 4)
+#define AM_STATUS_ECC_1_CORRECTED	(1 << 4)
+#define AM_STATUS_ECC_1_DETECTED	(2 << 4)
+#define AM_STATUS_ECC_MAX_CORRECTED	(3 << 4)
+
+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/alliancememory.c line 42

Quoted 10 lines

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

+			   SPINAND_PROG_LOAD(false, 0, NULL, 0));
+
+static int as5f34g04snd_ooblayout_ecc(struct mtd_info *mtd, int section,
+				      struct mtd_oob_region *region)
+{
+	if (section > 3)
+		return -ERANGE;
+
+	region->offset = 0x48;
+	region->length = 0x38;

Why would you have three sections if you have static offsets and length? That does not look correct.

drivers/mtd/nand/spi/alliancememory.c line 55

Quoted 13 lines

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

+
+	return 0;
+}
+
+static int as5f34g04snd_ooblayout_free(struct mtd_info *mtd, int section,
+				       struct mtd_oob_region *region)
+{
+	if (section)
+		return -ERANGE;
+
+	/*
+	 * It is unclear how many bytes are used for the bad block marker. We
+	 * reserve one byte here.

So far we reserved two bytes in Linux to mimic the raw NAND BBM.

Quoted 16 lines

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

+	 */
+
+	region->offset = 1;
+	region->length = 0x48 - 1;
+
+	return 0;
+}
+
+static const struct mtd_ooblayout_ops as5f34g04snd_ooblayout = {
+	.ecc = as5f34g04snd_ooblayout_ecc,
+	.free = as5f34g04snd_ooblayout_free,
+};
+
+static int am_ecc_get_status(struct spinand_device *spinand, u8 status)
+{
+	switch (status & AM_STATUS_ECC_BITMASK) {

drivers/mtd/nand/spi/alliancememory.c line 81

Quoted 10 lines

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

+	case AM_STATUS_ECC_NONE_DETECTED:
+		return 0;
+
+	case AM_STATUS_ECC_1_CORRECTED:
+		return 1;
+
+	case AM_STATUS_ECC_MAX_CORRECTED:
+		return 8;
+
+	case AM_STATUS_ECC_1_DETECTED:

What does this mean "1 detected"?

Quoted 19 lines

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

+		return -EBADMSG;
+
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+
+static const struct spinand_info alliancememory_spinand_table[] = {
+	SPINAND_INFO("AS5F34G04SND",
+		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x2f),
+		     NAND_MEMORG(1, 2048, 128, 64, 4096, 80, 1, 1, 1),
+		     NAND_ECCREQ(4, 512),
+		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
+					      &write_cache_variants,
+					      &update_cache_variants),
+		     SPINAND_HAS_QE_BIT,
+		     SPINAND_ECCINFO(&as5f34g04snd_ooblayout,

drivers/mtd/nand/spi/alliancememory.c line 110

Quoted 10 lines

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

+				     am_ecc_get_status)),
+};
+
+static int alliancememory_spinand_init(struct spinand_device *spinand)
+{
+	return 0;
+}
+
+static const struct spinand_manufacturer_ops alliancememory_spinand_manuf_ops = {
+	.init = alliancememory_spinand_init,

I don't think .init is mandatory, so if you do nothing in it, you can just drop it.

Quoted 26 lines

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

+};
+
+const struct spinand_manufacturer alliancememory_spinand_manufacturer = {
+	.id = SPINAND_MFR_ALLIANCEMEMORY,
+	.name = "AllianceMemory",
+	.chips = alliancememory_spinand_table,
+	.nchips = ARRAY_SIZE(alliancememory_spinand_table),
+	.ops = &alliancememory_spinand_manuf_ops,
+};

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

index dacd9c0e8b20..638391f77d8c 100644
@@ -937,6 +937,7 @@ static const struct nand_ops spinand_ops = {
 };
 
 static const struct spinand_manufacturer *spinand_manufacturers[] = {
+	&alliancememory_spinand_manufacturer,
 	&ato_spinand_manufacturer,
 	&gigadevice_spinand_manufacturer,
 	&macronix_spinand_manufacturer,

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

index 6d3392a7edc6..01be9f0f008a 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 alliancememory_spinand_manufacturer;
 extern const struct spinand_manufacturer ato_spinand_manufacturer;
 extern const struct spinand_manufacturer gigadevice_spinand_manufacturer;
 extern const struct spinand_manufacturer macronix_spinand_manufacturer;

Thanks,
Miquèl

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

Mario Kicherer

author

2023-01-25 12:13 UTC #2 lore raw

Hello Miquel,

Quoted 4 lines · Miquel Raynal

Quoted 4 lines
+static int as5f34g04snd_ooblayout_ecc(struct mtd_info *mtd, int section,
(quoted diff) +2 -0
+				      struct mtd_oob_region *region)
+{

Quoted 8 lines

drivers/mtd/nand/spi/alliancememory.c line 42
Quoted 5 lines
drivers/mtd/nand/spi/alliancememory.c new file +5 -0
+	if (section > 3)
+		return -ERANGE;
+
+	region->offset = 0x48;
+	region->length = 0x38;
Why would you have three sections if you have static offsets and length? That does not look correct.

Hm, I do not remember. I guess this is a leftover I forgot to remove.

Quoted 6 lines

drivers/mtd/nand/spi/alliancememory.c line 55
Quoted 4 lines
(quoted diff) +2 -0
+	/*
+	 * It is unclear how many bytes are used for the bad block marker. 
We
drivers/mtd/nand/spi/alliancememory.c new file +1 -0
+	 * reserve one byte here.
So far we reserved two bytes in Linux to mimic the raw NAND BBM.

Ok, changed.

Quoted 8 lines

Quoted 8 lines
+static int am_ecc_get_status(struct spinand_device *spinand, u8 status)
drivers/mtd/nand/spi/alliancememory.c new file +6 -0
+{
+	switch (status & AM_STATUS_ECC_BITMASK) {
+	case AM_STATUS_ECC_NONE_DETECTED:
+		return 0;
+
+	case AM_STATUS_ECC_1_CORRECTED:

Quoted 8 lines

drivers/mtd/nand/spi/alliancememory.c line 81
Quoted 6 lines
drivers/mtd/nand/spi/alliancememory.c new file +6 -0
+		return 1;
+
+	case AM_STATUS_ECC_MAX_CORRECTED:
+		return 8;
+
+	case AM_STATUS_ECC_1_DETECTED:
What does this mean "1 detected"?

According to the manual, the chip can report that a bit error was detected but could not be corrected.

Quoted 6 lines

drivers/mtd/nand/spi/alliancememory.c line 110
Quoted 3 lines
+static const struct spinand_manufacturer_ops
alliancememory_spinand_manuf_ops = {
drivers/mtd/nand/spi/alliancememory.c new file +1 -0
+	.init = alliancememory_spinand_init,
I don't think .init is mandatory, so if you do nothing in it, you can just drop it.

Done.

Thanks for the review!
Mario

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

Miquel Raynal

maintainer

2023-01-25 14:27 UTC #3 lore raw

Hi Mario,

[email protected] wrote on Wed, 25 Jan 2023 13:13:13 +0100:

Quoted 41 lines

Hello Miquel,
Quoted 11 lines · Miquel Raynal
drivers/mtd/nand/spi/alliancememory.c line 42
Quoted 8 lines
drivers/mtd/nand/spi/alliancememory.c new file +8 -0
+static int as5f34g04snd_ooblayout_ecc(struct mtd_info *mtd, int >> section,
+				      struct mtd_oob_region *region)
+{
+	if (section > 3)
+		return -ERANGE;
+
+	region->offset = 0x48;
+	region->length = 0x38;  
Why would you have three sections if you have static offsets and length? That does not look correct.
Hm, I do not remember. I guess this is a leftover I forgot to remove.
Quoted 5 lines
drivers/mtd/nand/spi/alliancememory.c line 55
Quoted 3 lines
drivers/mtd/nand/spi/alliancememory.c new file +3 -0
+	/*
+	 * It is unclear how many bytes are used for the bad block marker. >> We
+	 * reserve one byte here.  
So far we reserved two bytes in Linux to mimic the raw NAND BBM.
Ok, changed.
Quoted 7 lines
Quoted 7 lines
drivers/mtd/nand/spi/alliancememory.c new file +7 -0
+static int am_ecc_get_status(struct spinand_device *spinand, u8 >> status)
+{
+	switch (status & AM_STATUS_ECC_BITMASK) {
+	case AM_STATUS_ECC_NONE_DETECTED:
+		return 0;
+
+	case AM_STATUS_ECC_1_CORRECTED:
Quoted 8 lines
drivers/mtd/nand/spi/alliancememory.c line 81
Quoted 6 lines
drivers/mtd/nand/spi/alliancememory.c new file +6 -0
+		return 1;
+
+	case AM_STATUS_ECC_MAX_CORRECTED:
+		return 8;
+
+	case AM_STATUS_ECC_1_DETECTED:  
What does this mean "1 detected"?

Quoted 2 lines

According to the manual, the chip can report that a bit error was detected but could not be corrected.

That does not make a lot of sense. Either you use a Hamming algorithm, you will be able to correct 1 bit error and to detect 2 bit errors, or you use another algorithm (BCH, usually) with a strength of X (X > 1) and you'll be able to correct up to X errors and detect X+1 errors.

But only being able to detect a single bit flip without being able to correct it is strange (even useless?). So I don't understand how it should be used.

Quoted 11 lines

Quoted 6 lines
Review
Quoted 3 lines
(quoted diff) +2 -0
+static const struct spinand_manufacturer_ops >> alliancememory_spinand_manuf_ops = {
+	.init = alliancememory_spinand_init,  
I don't think .init is mandatory, so if you do nothing in it, you can just drop it.
Done.
Thanks for the review!
Mario

Thanks,
Miquèl

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

Mario Kicherer

author

2023-01-25 15:04 UTC #4 lore raw

Quoted 19 lines · Miquel Raynal

Quoted 16 lines
Quoted 7 lines
Quoted 7 lines
drivers/mtd/nand/spi/alliancememory.c new file +7 -0
+static int am_ecc_get_status(struct spinand_device *spinand, u8 >> status)
+{
+	switch (status & AM_STATUS_ECC_BITMASK) {
+	case AM_STATUS_ECC_NONE_DETECTED:
+		return 0;
+
+	case AM_STATUS_ECC_1_CORRECTED:
Quoted 8 lines
drivers/mtd/nand/spi/alliancememory.c line 81
Quoted 6 lines
drivers/mtd/nand/spi/alliancememory.c new file +6 -0
+		return 1;
+
+	case AM_STATUS_ECC_MAX_CORRECTED:
+		return 8;
+
+	case AM_STATUS_ECC_1_DETECTED:
What does this mean "1 detected"?
Quoted 2 lines
According to the manual, the chip can report that a bit error was detected but could not be corrected.

Quoted 8 lines

That does not make a lot of sense. Either you use a Hamming algorithm, you will be able to correct 1 bit error and to detect 2 bit errors, or you use another algorithm (BCH, usually) with a strength of X (X > 1) and you'll be able to correct up to X errors and detect X+1 errors.
But only being able to detect a single bit flip without being able to correct it is strange (even useless?). So I don't understand how it should be used.

Unfortunately, I do not have much experience with NAND flashes. The manual does not say much more - page 41 in [1] in case you want to see for yourself. I thought returning -EBADMSG in this case would be the right thing to do as an uncorrectable error happened.

I have not used the flash much yet, so I cannot say that I have much confidence into the chip's error reporting. So far the flash worked without issues.

Should I change something in the patch?

Best regards,
Mario

[1] https://www.alliancememory.com/wp-content/uploads/pdf/flash/AllianceMemory_SPI_NAND_Flash_July2020_Rev1.0.pdf

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

Miquel Raynal

maintainer

2023-01-26 08:53 UTC #5 lore raw

Hi Mario,

[email protected] wrote on Wed, 25 Jan 2023 16:03:58 +0100:

Quoted 24 lines · Miquel Raynal

Quoted 23 lines
Quoted 15 lines
Quoted 15 lines
drivers/mtd/nand/spi/alliancememory.c line 81
Quoted 13 lines
drivers/mtd/nand/spi/alliancememory.c new file +13 -0
+static int am_ecc_get_status(struct spinand_device *spinand, u8 >> status)
+{
+	switch (status & AM_STATUS_ECC_BITMASK) {
+	case AM_STATUS_ECC_NONE_DETECTED:
+		return 0;
+
+	case AM_STATUS_ECC_1_CORRECTED:
+		return 1;
+
+	case AM_STATUS_ECC_MAX_CORRECTED:
+		return 8;
+
+	case AM_STATUS_ECC_1_DETECTED:  
What does this mean "1 detected"?
Quoted 2 lines
Quoted 1 line
Quoted 1 line
According to the manual, the chip can report that a bit error was >> detected
but could not be corrected.
That does not make a lot of sense. Either you use a Hamming algorithm, you will be able to correct 1 bit error and to detect 2 bit errors, or you use another algorithm (BCH, usually) with a strength of X (X > 1) and you'll be able to correct up to X errors and detect X+1 errors.

Quoted 8 lines

Quoted 3 lines
But only being able to detect a single bit flip without being able to correct it is strange (even useless?). So I don't understand how it should be used.
Unfortunately, I do not have much experience with NAND flashes. The manual does not say much more - page 41 in [1] in case you want to see for yourself. I thought returning -EBADMSG in this case would be the right thing to do as an uncorrectable error happened.

00b = No bit errors were detected
01b = bit error was detected and corrected
10b = bit error was detected and not corrected
11b = bit error was detected and corrected, error bit number = ECC max which is according to extended register.

I guess it means that if you have a strength of X:
- from 1 bf to X-1 the status will be set to 0x1
- if you have X bf it will be set to 0x3
- if you have more than X bf, it will be set to 0x2.

Please update the code and use mtd-utils (like the bit error test) to verify what the tools says and by tracing the register values extracted with a simple printk to follow if the above logic works.

Quoted 10 lines

I have not used the flash much yet, so I cannot say that I have much confidence into the chip's error reporting. So far the flash worked without issues.
Should I change something in the patch?
Best regards,
Mario
[1] https://www.alliancememory.com/wp-content/uploads/pdf/flash/AllianceMemory_SPI_NAND_Flash_July2020_Rev1.0.pdf

Thanks,
Miquèl

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

Mario Kicherer

author

2023-01-26 14:35 UTC #6 lore raw

Hello Miquèl,

I made all requested changes and added additional code to make the ooblayout functions compatible with all flashes of this series.

I also ran nandbiterrs with the following result:

# nandbiterrs -i /dev/mtd11
incremental biterrors test
Successfully corrected 0 bit errors per subpage
Inserted biterror @ 0/5
Read reported 7 corrected bit errors
Successfully corrected 1 bit errors per subpage
Inserted biterror @ 0/2
Read reported 7 corrected bit errors
Successfully corrected 2 bit errors per subpage
Inserted biterror @ 0/0
Read reported 7 corrected bit errors
Successfully corrected 3 bit errors per subpage
Inserted biterror @ 1/7
Read reported 7 corrected bit errors
Successfully corrected 4 bit errors per subpage
Inserted biterror @ 1/5
Read reported 7 corrected bit errors
Successfully corrected 5 bit errors per subpage
Inserted biterror @ 1/2
Read reported 7 corrected bit errors
Successfully corrected 6 bit errors per subpage
Inserted biterror @ 1/0
Read reported 7 corrected bit errors
Successfully corrected 7 bit errors per subpage
Inserted biterror @ 2/6
Read reported 8 corrected bit errors
Successfully corrected 8 bit errors per subpage
Inserted biterror @ 2/5
Failed to recover 1 bitflips
Read error after 9 bit errors per page

I would say this looks good?

I will send the new patch in a few seconds.

Thank you!
Mario

Quoted 58 lines · Miquel Raynal

Hi Mario,
[email protected] wrote on Wed, 25 Jan 2023 16:03:58 +0100:
Quoted 24 lines · Miquel Raynal
Quoted 23 lines
Quoted 15 lines
Quoted 15 lines
drivers/mtd/nand/spi/alliancememory.c line 81
Quoted 13 lines
drivers/mtd/nand/spi/alliancememory.c new file +13 -0
+static int am_ecc_get_status(struct spinand_device *spinand, u8 >> status)
+{
+	switch (status & AM_STATUS_ECC_BITMASK) {
+	case AM_STATUS_ECC_NONE_DETECTED:
+		return 0;
+
+	case AM_STATUS_ECC_1_CORRECTED:
+		return 1;
+
+	case AM_STATUS_ECC_MAX_CORRECTED:
+		return 8;
+
+	case AM_STATUS_ECC_1_DETECTED:
What does this mean "1 detected"?
Quoted 2 lines
Quoted 1 line
Quoted 1 line
According to the manual, the chip can report that a bit error was >> detected
but could not be corrected.
That does not make a lot of sense. Either you use a Hamming algorithm, you will be able to correct 1 bit error and to detect 2 bit errors, or you use another algorithm (BCH, usually) with a strength of X (X > 1) and you'll be able to correct up to X errors and detect X+1 errors.
Quoted 8 lines
Quoted 3 lines
But only being able to detect a single bit flip without being able to correct it is strange (even useless?). So I don't understand how it should be used.
Unfortunately, I do not have much experience with NAND flashes. The manual does not say much more - page 41 in [1] in case you want to see for yourself. I thought returning -EBADMSG in this case would be the right thing to do as an uncorrectable error happened.
00b = No bit errors were detected
01b = bit error was detected and corrected
10b = bit error was detected and not corrected
11b = bit error was detected and corrected, error bit number = ECC max which is according to extended register.
I guess it means that if you have a strength of X:
- from 1 bf to X-1 the status will be set to 0x1
- if you have X bf it will be set to 0x3
- if you have more than X bf, it will be set to 0x2.
Please update the code and use mtd-utils (like the bit error test) to verify what the tools says and by tracing the register values extracted with a simple printk to follow if the above logic works.
Quoted 6 lines
I have not used the flash much yet, so I cannot say that I have much confidence into the chip's error reporting. So far the flash worked without issues.
Should I change something in the patch?

Quoted 8 lines

Quoted 4 lines
Best regards,
Mario
[1] https://www.alliancememory.com/wp-content/uploads/pdf/flash/AllianceMemory_SPI_NAND_Flash_July2020_Rev1.0.pdf
Thanks,
Miquèl

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

Miquel Raynal

maintainer

2023-01-26 14:58 UTC #7 lore raw

Hello,

[email protected] wrote on Thu, 26 Jan 2023 15:34:47 +0100:

Quoted 5 lines

Hello Miquèl,
I made all requested changes and added additional code to make the ooblayout functions compatible with all flashes of this series.

Quoted 8 lines

I also ran nandbiterrs with the following result:

# nandbiterrs -i /dev/mtd11
incremental biterrors test
Successfully corrected 0 bit errors per subpage
Inserted biterror @ 0/5
Read reported 7 corrected bit errors
Successfully corrected 1 bit errors per subpage

Yeah, well, I guess we don't have a choice. UBI would see this as a need to rewrite the block somewhere else because of the too high amount of bit flips. UBI's threshold is somewhere around 3/4 or the strength, so these flashes could rapidly wear out, but I don't think we have another choice.

Quoted 25 lines

Inserted biterror @ 0/2
Read reported 7 corrected bit errors
Successfully corrected 2 bit errors per subpage
Inserted biterror @ 0/0
Read reported 7 corrected bit errors
Successfully corrected 3 bit errors per subpage
Inserted biterror @ 1/7
Read reported 7 corrected bit errors
Successfully corrected 4 bit errors per subpage
Inserted biterror @ 1/5
Read reported 7 corrected bit errors
Successfully corrected 5 bit errors per subpage
Inserted biterror @ 1/2
Read reported 7 corrected bit errors
Successfully corrected 6 bit errors per subpage
Inserted biterror @ 1/0
Read reported 7 corrected bit errors
Successfully corrected 7 bit errors per subpage
Inserted biterror @ 2/6
Read reported 8 corrected bit errors
Successfully corrected 8 bit errors per subpage
Inserted biterror @ 2/5
Failed to recover 1 bitflips
Read error after 9 bit errors per page

Quoted 7 lines

I would say this looks good?
I will send the new patch in a few seconds.
Thank you!
Mario

Thanks,
Miquèl

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