* [PATCH 0/2] mtd: spi-nand: Add support for read retry @ 2024-09-05 5:53 ` Cheng Ming Lin 0 siblings, 0 replies; 23+ messages in thread From: Cheng Ming Lin @ 2024-09-05 5:53 UTC (permalink / raw) To: miquel.raynal, vigneshr, linux-mtd, linux-kernel Cc: richard, alvinzhou, leoyu, Cheng Ming Lin From: Cheng Ming Lin <chengminglin@mxic.com.tw> When the host ECC fails to correct the data error of NAND device, there's a special read for data recovery method which host executes the Special Read for Data Recovery operation and may recover the lost data by host ECC again. For more detailed information, please refer to page 27 in the link below: Link: https://www.macronix.com/Lists/Datasheet/Attachments/9034/MX35LF1G24AD,%203V,%201Gb,%20v1.4.pdf Cheng Ming Lin (2): mtd: spi-nand: Add fixups for read retry mtd: spi-nand: Add read retry support drivers/mtd/nand/spi/core.c | 33 +++++++++++++- drivers/mtd/nand/spi/macronix.c | 79 ++++++++++++++++++++++++++------- include/linux/mtd/spinand.h | 17 +++++++ 3 files changed, 112 insertions(+), 17 deletions(-) -- 2.25.1 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 0/2] mtd: spi-nand: Add support for read retry @ 2024-09-05 5:53 ` Cheng Ming Lin 0 siblings, 0 replies; 23+ messages in thread From: Cheng Ming Lin @ 2024-09-05 5:53 UTC (permalink / raw) To: miquel.raynal, vigneshr, linux-mtd, linux-kernel Cc: richard, alvinzhou, leoyu, Cheng Ming Lin From: Cheng Ming Lin <chengminglin@mxic.com.tw> When the host ECC fails to correct the data error of NAND device, there's a special read for data recovery method which host executes the Special Read for Data Recovery operation and may recover the lost data by host ECC again. For more detailed information, please refer to page 27 in the link below: Link: https://www.macronix.com/Lists/Datasheet/Attachments/9034/MX35LF1G24AD,%203V,%201Gb,%20v1.4.pdf Cheng Ming Lin (2): mtd: spi-nand: Add fixups for read retry mtd: spi-nand: Add read retry support drivers/mtd/nand/spi/core.c | 33 +++++++++++++- drivers/mtd/nand/spi/macronix.c | 79 ++++++++++++++++++++++++++------- include/linux/mtd/spinand.h | 17 +++++++ 3 files changed, 112 insertions(+), 17 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/2] mtd: spi-nand: Add fixups for read retry 2024-09-05 5:53 ` Cheng Ming Lin @ 2024-09-05 5:53 ` Cheng Ming Lin -1 siblings, 0 replies; 23+ messages in thread From: Cheng Ming Lin @ 2024-09-05 5:53 UTC (permalink / raw) To: miquel.raynal, vigneshr, linux-mtd, linux-kernel Cc: richard, alvinzhou, leoyu, Cheng Ming Lin From: Cheng Ming Lin <chengminglin@mxic.com.tw> Add fixups for support read retry: - Initialize the NAND device maximum retry mode. - Set feature on Special Read for Data Recovery register. The Special Read for Data Recovery operation is enabled by Set Feature function. There are 5 modes for the user to recover the lost data. Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw> --- drivers/mtd/nand/spi/macronix.c | 79 ++++++++++++++++++++++++++------- include/linux/mtd/spinand.h | 17 +++++++ 2 files changed, 81 insertions(+), 15 deletions(-) diff --git a/drivers/mtd/nand/spi/macronix.c b/drivers/mtd/nand/spi/macronix.c index 3f9e9c572854..baca67ff1cd6 100644 --- a/drivers/mtd/nand/spi/macronix.c +++ b/drivers/mtd/nand/spi/macronix.c @@ -9,6 +9,8 @@ #include <linux/kernel.h> #include <linux/mtd/spinand.h> +#define MACRONIX_NUM_READ_RETRY_MODES 6 +#define MACRONIX_FEATURE_ADDR_READ_RETRY 0x70 #define SPINAND_MFR_MACRONIX 0xC2 #define MACRONIX_ECCSR_MASK 0x0F @@ -100,6 +102,38 @@ static int mx35lf1ge4ab_ecc_get_status(struct spinand_device *spinand, return -EINVAL; } +/** + * macronix_spinand_init_read_retry - Initialize read_retries + * @spinand: SPI NAND device + * + * Return: the number of read retry modes + */ +static int macronix_spinand_init_read_retry(struct spinand_device *spinand) +{ + return MACRONIX_NUM_READ_RETRY_MODES; +} + +/** + * macronix_spinand_setup_read_retry - Set the retry mode + * @spinand: SPI NAND device + * @retry_mode: Specify which retry mode to set + * + * Return: 0 on success, -error otherwise + */ +static int macronix_spinand_setup_read_retry(struct spinand_device *spinand, u8 retry_mode) +{ + struct spi_mem_op op = SPINAND_SET_FEATURE_OP(MACRONIX_FEATURE_ADDR_READ_RETRY, + spinand->scratchbuf); + + *spinand->scratchbuf = retry_mode; + return spi_mem_exec_op(spinand->spimem, &op); +} + +static const struct spi_nand_fixups read_retry_fixups = { + .init_read_retry = macronix_spinand_init_read_retry, + .setup_read_retry = macronix_spinand_setup_read_retry, +}; + static const struct spinand_info macronix_spinand_table[] = { SPINAND_INFO("MX35LF1GE4AB", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x12), @@ -129,7 +163,8 @@ static const struct spinand_info macronix_spinand_table[] = { &update_cache_variants), SPINAND_HAS_QE_BIT, SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, - mx35lf1ge4ab_ecc_get_status)), + mx35lf1ge4ab_ecc_get_status), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35LF4GE4AD", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x37, 0x03), NAND_MEMORG(1, 4096, 128, 64, 2048, 40, 1, 1, 1), @@ -139,7 +174,8 @@ static const struct spinand_info macronix_spinand_table[] = { &update_cache_variants), SPINAND_HAS_QE_BIT, SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, - mx35lf1ge4ab_ecc_get_status)), + mx35lf1ge4ab_ecc_get_status), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35LF1G24AD", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x14, 0x03), NAND_MEMORG(1, 2048, 128, 64, 1024, 20, 1, 1, 1), @@ -148,7 +184,8 @@ static const struct spinand_info macronix_spinand_table[] = { &write_cache_variants, &update_cache_variants), SPINAND_HAS_QE_BIT, - SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, NULL)), + SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, NULL), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35LF2G24AD", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x24, 0x03), NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 2, 1, 1), @@ -157,7 +194,8 @@ static const struct spinand_info macronix_spinand_table[] = { &write_cache_variants, &update_cache_variants), SPINAND_HAS_QE_BIT, - SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, NULL)), + SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, NULL), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35LF2G24AD-Z4I8", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x64, 0x03), NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 1, 1, 1), @@ -166,7 +204,8 @@ static const struct spinand_info macronix_spinand_table[] = { &write_cache_variants, &update_cache_variants), SPINAND_HAS_QE_BIT, - SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, NULL)), + SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, NULL), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35LF4G24AD", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x35, 0x03), NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 2, 1, 1), @@ -175,7 +214,8 @@ static const struct spinand_info macronix_spinand_table[] = { &write_cache_variants, &update_cache_variants), SPINAND_HAS_QE_BIT, - SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, NULL)), + SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, NULL), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35LF4G24AD-Z4I8", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x75, 0x03), NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1), @@ -184,7 +224,8 @@ static const struct spinand_info macronix_spinand_table[] = { &write_cache_variants, &update_cache_variants), SPINAND_HAS_QE_BIT, - SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, NULL)), + SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, NULL), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX31LF1GE4BC", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x1e), NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1), @@ -225,7 +266,8 @@ static const struct spinand_info macronix_spinand_table[] = { &update_cache_variants), SPINAND_HAS_QE_BIT, SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, - mx35lf1ge4ab_ecc_get_status)), + mx35lf1ge4ab_ecc_get_status), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35UF4G24AD-Z4I8", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xf5, 0x03), NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1), @@ -235,7 +277,8 @@ static const struct spinand_info macronix_spinand_table[] = { &update_cache_variants), SPINAND_HAS_QE_BIT, SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, - mx35lf1ge4ab_ecc_get_status)), + mx35lf1ge4ab_ecc_get_status), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35UF4GE4AD", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xb7, 0x03), NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1), @@ -245,7 +288,8 @@ static const struct spinand_info macronix_spinand_table[] = { &update_cache_variants), SPINAND_HAS_QE_BIT, SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, - mx35lf1ge4ab_ecc_get_status)), + mx35lf1ge4ab_ecc_get_status), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35UF2G14AC", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xa0), NAND_MEMORG(1, 2048, 64, 64, 2048, 40, 2, 1, 1), @@ -265,7 +309,8 @@ static const struct spinand_info macronix_spinand_table[] = { &update_cache_variants), SPINAND_HAS_QE_BIT, SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, - mx35lf1ge4ab_ecc_get_status)), + mx35lf1ge4ab_ecc_get_status), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35UF2G24AD-Z4I8", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xe4, 0x03), NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 1, 1, 1), @@ -275,7 +320,8 @@ static const struct spinand_info macronix_spinand_table[] = { &update_cache_variants), SPINAND_HAS_QE_BIT, SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, - mx35lf1ge4ab_ecc_get_status)), + mx35lf1ge4ab_ecc_get_status), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35UF2GE4AD", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xa6, 0x03), NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 1, 1, 1), @@ -285,7 +331,8 @@ static const struct spinand_info macronix_spinand_table[] = { &update_cache_variants), SPINAND_HAS_QE_BIT, SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, - mx35lf1ge4ab_ecc_get_status)), + mx35lf1ge4ab_ecc_get_status), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35UF2GE4AC", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xa2, 0x01), NAND_MEMORG(1, 2048, 64, 64, 2048, 40, 1, 1, 1), @@ -315,7 +362,8 @@ static const struct spinand_info macronix_spinand_table[] = { &update_cache_variants), SPINAND_HAS_QE_BIT, SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, - mx35lf1ge4ab_ecc_get_status)), + mx35lf1ge4ab_ecc_get_status), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35UF1GE4AD", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x96, 0x03), NAND_MEMORG(1, 2048, 128, 64, 1024, 20, 1, 1, 1), @@ -325,7 +373,8 @@ static const struct spinand_info macronix_spinand_table[] = { &update_cache_variants), SPINAND_HAS_QE_BIT, SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, - mx35lf1ge4ab_ecc_get_status)), + mx35lf1ge4ab_ecc_get_status), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35UF1GE4AC", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x92, 0x01), NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1), diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h index 5c19ead60499..e567d00a2805 100644 --- a/include/linux/mtd/spinand.h +++ b/include/linux/mtd/spinand.h @@ -354,6 +354,7 @@ struct spinand_info { } op_variants; int (*select_target)(struct spinand_device *spinand, unsigned int target); + const struct spi_nand_fixups *fixups; }; #define SPINAND_ID(__method, ...) \ @@ -379,6 +380,9 @@ struct spinand_info { #define SPINAND_SELECT_TARGET(__func) \ .select_target = __func, +#define SPINAND_FIXUPS(__func) \ + .fixups = __func, + #define SPINAND_INFO(__model, __id, __memorg, __eccreq, __op_variants, \ __flags, ...) \ { \ @@ -398,6 +402,16 @@ struct spinand_dirmap { struct spi_mem_dirmap_desc *rdesc_ecc; }; +/** + * struct spi_nand_fixups - SPI NAND fixup hooks + * @init_read_retry: initialize spinand->read_retries + * @setup_read_retry: set the retry mode + */ +struct spi_nand_fixups { + int (*init_read_retry)(struct spinand_device *spinand); + int (*setup_read_retry)(struct spinand_device *spinand, u8 retry_mode); +}; + /** * struct spinand_device - SPI NAND device instance * @base: NAND device instance @@ -423,6 +437,7 @@ struct spinand_dirmap { * the stack * @manufacturer: SPI NAND manufacturer information * @priv: manufacturer private data + * @read_retries: the number of read retry modes supported */ struct spinand_device { struct nand_device base; @@ -449,8 +464,10 @@ struct spinand_device { u8 *databuf; u8 *oobbuf; u8 *scratchbuf; + const struct spinand_info *info; const struct spinand_manufacturer *manufacturer; void *priv; + int read_retries; }; /** -- 2.25.1 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 1/2] mtd: spi-nand: Add fixups for read retry @ 2024-09-05 5:53 ` Cheng Ming Lin 0 siblings, 0 replies; 23+ messages in thread From: Cheng Ming Lin @ 2024-09-05 5:53 UTC (permalink / raw) To: miquel.raynal, vigneshr, linux-mtd, linux-kernel Cc: richard, alvinzhou, leoyu, Cheng Ming Lin From: Cheng Ming Lin <chengminglin@mxic.com.tw> Add fixups for support read retry: - Initialize the NAND device maximum retry mode. - Set feature on Special Read for Data Recovery register. The Special Read for Data Recovery operation is enabled by Set Feature function. There are 5 modes for the user to recover the lost data. Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw> --- drivers/mtd/nand/spi/macronix.c | 79 ++++++++++++++++++++++++++------- include/linux/mtd/spinand.h | 17 +++++++ 2 files changed, 81 insertions(+), 15 deletions(-) diff --git a/drivers/mtd/nand/spi/macronix.c b/drivers/mtd/nand/spi/macronix.c index 3f9e9c572854..baca67ff1cd6 100644 --- a/drivers/mtd/nand/spi/macronix.c +++ b/drivers/mtd/nand/spi/macronix.c @@ -9,6 +9,8 @@ #include <linux/kernel.h> #include <linux/mtd/spinand.h> +#define MACRONIX_NUM_READ_RETRY_MODES 6 +#define MACRONIX_FEATURE_ADDR_READ_RETRY 0x70 #define SPINAND_MFR_MACRONIX 0xC2 #define MACRONIX_ECCSR_MASK 0x0F @@ -100,6 +102,38 @@ static int mx35lf1ge4ab_ecc_get_status(struct spinand_device *spinand, return -EINVAL; } +/** + * macronix_spinand_init_read_retry - Initialize read_retries + * @spinand: SPI NAND device + * + * Return: the number of read retry modes + */ +static int macronix_spinand_init_read_retry(struct spinand_device *spinand) +{ + return MACRONIX_NUM_READ_RETRY_MODES; +} + +/** + * macronix_spinand_setup_read_retry - Set the retry mode + * @spinand: SPI NAND device + * @retry_mode: Specify which retry mode to set + * + * Return: 0 on success, -error otherwise + */ +static int macronix_spinand_setup_read_retry(struct spinand_device *spinand, u8 retry_mode) +{ + struct spi_mem_op op = SPINAND_SET_FEATURE_OP(MACRONIX_FEATURE_ADDR_READ_RETRY, + spinand->scratchbuf); + + *spinand->scratchbuf = retry_mode; + return spi_mem_exec_op(spinand->spimem, &op); +} + +static const struct spi_nand_fixups read_retry_fixups = { + .init_read_retry = macronix_spinand_init_read_retry, + .setup_read_retry = macronix_spinand_setup_read_retry, +}; + static const struct spinand_info macronix_spinand_table[] = { SPINAND_INFO("MX35LF1GE4AB", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x12), @@ -129,7 +163,8 @@ static const struct spinand_info macronix_spinand_table[] = { &update_cache_variants), SPINAND_HAS_QE_BIT, SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, - mx35lf1ge4ab_ecc_get_status)), + mx35lf1ge4ab_ecc_get_status), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35LF4GE4AD", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x37, 0x03), NAND_MEMORG(1, 4096, 128, 64, 2048, 40, 1, 1, 1), @@ -139,7 +174,8 @@ static const struct spinand_info macronix_spinand_table[] = { &update_cache_variants), SPINAND_HAS_QE_BIT, SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, - mx35lf1ge4ab_ecc_get_status)), + mx35lf1ge4ab_ecc_get_status), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35LF1G24AD", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x14, 0x03), NAND_MEMORG(1, 2048, 128, 64, 1024, 20, 1, 1, 1), @@ -148,7 +184,8 @@ static const struct spinand_info macronix_spinand_table[] = { &write_cache_variants, &update_cache_variants), SPINAND_HAS_QE_BIT, - SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, NULL)), + SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, NULL), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35LF2G24AD", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x24, 0x03), NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 2, 1, 1), @@ -157,7 +194,8 @@ static const struct spinand_info macronix_spinand_table[] = { &write_cache_variants, &update_cache_variants), SPINAND_HAS_QE_BIT, - SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, NULL)), + SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, NULL), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35LF2G24AD-Z4I8", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x64, 0x03), NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 1, 1, 1), @@ -166,7 +204,8 @@ static const struct spinand_info macronix_spinand_table[] = { &write_cache_variants, &update_cache_variants), SPINAND_HAS_QE_BIT, - SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, NULL)), + SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, NULL), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35LF4G24AD", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x35, 0x03), NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 2, 1, 1), @@ -175,7 +214,8 @@ static const struct spinand_info macronix_spinand_table[] = { &write_cache_variants, &update_cache_variants), SPINAND_HAS_QE_BIT, - SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, NULL)), + SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, NULL), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35LF4G24AD-Z4I8", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x75, 0x03), NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1), @@ -184,7 +224,8 @@ static const struct spinand_info macronix_spinand_table[] = { &write_cache_variants, &update_cache_variants), SPINAND_HAS_QE_BIT, - SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, NULL)), + SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, NULL), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX31LF1GE4BC", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x1e), NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1), @@ -225,7 +266,8 @@ static const struct spinand_info macronix_spinand_table[] = { &update_cache_variants), SPINAND_HAS_QE_BIT, SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, - mx35lf1ge4ab_ecc_get_status)), + mx35lf1ge4ab_ecc_get_status), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35UF4G24AD-Z4I8", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xf5, 0x03), NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1), @@ -235,7 +277,8 @@ static const struct spinand_info macronix_spinand_table[] = { &update_cache_variants), SPINAND_HAS_QE_BIT, SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, - mx35lf1ge4ab_ecc_get_status)), + mx35lf1ge4ab_ecc_get_status), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35UF4GE4AD", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xb7, 0x03), NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1), @@ -245,7 +288,8 @@ static const struct spinand_info macronix_spinand_table[] = { &update_cache_variants), SPINAND_HAS_QE_BIT, SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, - mx35lf1ge4ab_ecc_get_status)), + mx35lf1ge4ab_ecc_get_status), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35UF2G14AC", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xa0), NAND_MEMORG(1, 2048, 64, 64, 2048, 40, 2, 1, 1), @@ -265,7 +309,8 @@ static const struct spinand_info macronix_spinand_table[] = { &update_cache_variants), SPINAND_HAS_QE_BIT, SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, - mx35lf1ge4ab_ecc_get_status)), + mx35lf1ge4ab_ecc_get_status), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35UF2G24AD-Z4I8", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xe4, 0x03), NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 1, 1, 1), @@ -275,7 +320,8 @@ static const struct spinand_info macronix_spinand_table[] = { &update_cache_variants), SPINAND_HAS_QE_BIT, SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, - mx35lf1ge4ab_ecc_get_status)), + mx35lf1ge4ab_ecc_get_status), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35UF2GE4AD", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xa6, 0x03), NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 1, 1, 1), @@ -285,7 +331,8 @@ static const struct spinand_info macronix_spinand_table[] = { &update_cache_variants), SPINAND_HAS_QE_BIT, SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, - mx35lf1ge4ab_ecc_get_status)), + mx35lf1ge4ab_ecc_get_status), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35UF2GE4AC", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xa2, 0x01), NAND_MEMORG(1, 2048, 64, 64, 2048, 40, 1, 1, 1), @@ -315,7 +362,8 @@ static const struct spinand_info macronix_spinand_table[] = { &update_cache_variants), SPINAND_HAS_QE_BIT, SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, - mx35lf1ge4ab_ecc_get_status)), + mx35lf1ge4ab_ecc_get_status), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35UF1GE4AD", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x96, 0x03), NAND_MEMORG(1, 2048, 128, 64, 1024, 20, 1, 1, 1), @@ -325,7 +373,8 @@ static const struct spinand_info macronix_spinand_table[] = { &update_cache_variants), SPINAND_HAS_QE_BIT, SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, - mx35lf1ge4ab_ecc_get_status)), + mx35lf1ge4ab_ecc_get_status), + SPINAND_FIXUPS(&read_retry_fixups)), SPINAND_INFO("MX35UF1GE4AC", SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x92, 0x01), NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1), diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h index 5c19ead60499..e567d00a2805 100644 --- a/include/linux/mtd/spinand.h +++ b/include/linux/mtd/spinand.h @@ -354,6 +354,7 @@ struct spinand_info { } op_variants; int (*select_target)(struct spinand_device *spinand, unsigned int target); + const struct spi_nand_fixups *fixups; }; #define SPINAND_ID(__method, ...) \ @@ -379,6 +380,9 @@ struct spinand_info { #define SPINAND_SELECT_TARGET(__func) \ .select_target = __func, +#define SPINAND_FIXUPS(__func) \ + .fixups = __func, + #define SPINAND_INFO(__model, __id, __memorg, __eccreq, __op_variants, \ __flags, ...) \ { \ @@ -398,6 +402,16 @@ struct spinand_dirmap { struct spi_mem_dirmap_desc *rdesc_ecc; }; +/** + * struct spi_nand_fixups - SPI NAND fixup hooks + * @init_read_retry: initialize spinand->read_retries + * @setup_read_retry: set the retry mode + */ +struct spi_nand_fixups { + int (*init_read_retry)(struct spinand_device *spinand); + int (*setup_read_retry)(struct spinand_device *spinand, u8 retry_mode); +}; + /** * struct spinand_device - SPI NAND device instance * @base: NAND device instance @@ -423,6 +437,7 @@ struct spinand_dirmap { * the stack * @manufacturer: SPI NAND manufacturer information * @priv: manufacturer private data + * @read_retries: the number of read retry modes supported */ struct spinand_device { struct nand_device base; @@ -449,8 +464,10 @@ struct spinand_device { u8 *databuf; u8 *oobbuf; u8 *scratchbuf; + const struct spinand_info *info; const struct spinand_manufacturer *manufacturer; void *priv; + int read_retries; }; /** -- 2.25.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nand: Add fixups for read retry 2024-09-05 5:53 ` Cheng Ming Lin @ 2024-10-01 9:40 ` Miquel Raynal -1 siblings, 0 replies; 23+ messages in thread From: Miquel Raynal @ 2024-10-01 9:40 UTC (permalink / raw) To: Cheng Ming Lin Cc: vigneshr, linux-mtd, linux-kernel, richard, alvinzhou, leoyu, Cheng Ming Lin Hi Cheng Ming, linchengming884@gmail.com wrote on Thu, 5 Sep 2024 13:53:32 +0800: > From: Cheng Ming Lin <chengminglin@mxic.com.tw> > > Add fixups for support read retry: > - Initialize the NAND device maximum retry mode. > - Set feature on Special Read for Data Recovery register. > > The Special Read for Data Recovery operation is enabled by Set Feature > function. > > There are 5 modes for the user to recover the lost data. > > Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw> > --- > drivers/mtd/nand/spi/macronix.c | 79 ++++++++++++++++++++++++++------- > include/linux/mtd/spinand.h | 17 +++++++ > 2 files changed, 81 insertions(+), 15 deletions(-) > > diff --git a/drivers/mtd/nand/spi/macronix.c b/drivers/mtd/nand/spi/macronix.c > index 3f9e9c572854..baca67ff1cd6 100644 > --- a/drivers/mtd/nand/spi/macronix.c > +++ b/drivers/mtd/nand/spi/macronix.c > @@ -9,6 +9,8 @@ > #include <linux/kernel.h> > #include <linux/mtd/spinand.h> > > +#define MACRONIX_NUM_READ_RETRY_MODES 6 You said 5 in the cover letter? > +#define MACRONIX_FEATURE_ADDR_READ_RETRY 0x70 Both definitions should probably come... > #define SPINAND_MFR_MACRONIX 0xC2 > #define MACRONIX_ECCSR_MASK 0x0F ...here > > @@ -100,6 +102,38 @@ static int mx35lf1ge4ab_ecc_get_status(struct spinand_device *spinand, > return -EINVAL; > } > > +/** > + * macronix_spinand_init_read_retry - Initialize read_retries > + * @spinand: SPI NAND device > + * > + * Return: the number of read retry modes > + */ > +static int macronix_spinand_init_read_retry(struct spinand_device *spinand) > +{ > + return MACRONIX_NUM_READ_RETRY_MODES; Does not sound very useful as a function? > +} > + > +/** > + * macronix_spinand_setup_read_retry - Set the retry mode > + * @spinand: SPI NAND device > + * @retry_mode: Specify which retry mode to set > + * > + * Return: 0 on success, -error otherwise , a negative error code otherwise. > + */ > +static int macronix_spinand_setup_read_retry(struct spinand_device *spinand, u8 retry_mode) > +{ > + struct spi_mem_op op = SPINAND_SET_FEATURE_OP(MACRONIX_FEATURE_ADDR_READ_RETRY, > + spinand->scratchbuf); > + > + *spinand->scratchbuf = retry_mode; > + return spi_mem_exec_op(spinand->spimem, &op); > +} > + > +static const struct spi_nand_fixups read_retry_fixups = { > + .init_read_retry = macronix_spinand_init_read_retry, > + .setup_read_retry = macronix_spinand_setup_read_retry, > +}; > + ... > @@ -325,7 +373,8 @@ static const struct spinand_info macronix_spinand_table[] = { > &update_cache_variants), > SPINAND_HAS_QE_BIT, > SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, > - mx35lf1ge4ab_ecc_get_status)), > + mx35lf1ge4ab_ecc_get_status), > + SPINAND_FIXUPS(&read_retry_fixups)), > SPINAND_INFO("MX35UF1GE4AC", > SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x92, 0x01), > NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1), I expect a patch targeting the core first, and then the changes in the Macronix driver. > diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h > index 5c19ead60499..e567d00a2805 100644 > --- a/include/linux/mtd/spinand.h > +++ b/include/linux/mtd/spinand.h > @@ -354,6 +354,7 @@ struct spinand_info { > } op_variants; > int (*select_target)(struct spinand_device *spinand, > unsigned int target); > + const struct spi_nand_fixups *fixups; > }; > > #define SPINAND_ID(__method, ...) \ > @@ -379,6 +380,9 @@ struct spinand_info { > #define SPINAND_SELECT_TARGET(__func) \ > .select_target = __func, > > +#define SPINAND_FIXUPS(__func) \ > + .fixups = __func, I don't like the fixups naming. It feels like something is inherently wrong. > + > #define SPINAND_INFO(__model, __id, __memorg, __eccreq, __op_variants, \ > __flags, ...) \ > { \ > @@ -398,6 +402,16 @@ struct spinand_dirmap { > struct spi_mem_dirmap_desc *rdesc_ecc; > }; > > +/** > + * struct spi_nand_fixups - SPI NAND fixup hooks > + * @init_read_retry: initialize spinand->read_retries > + * @setup_read_retry: set the retry mode > + */ > +struct spi_nand_fixups { > + int (*init_read_retry)(struct spinand_device *spinand); > + int (*setup_read_retry)(struct spinand_device *spinand, u8 retry_mode); > +}; > + > /** > * struct spinand_device - SPI NAND device instance > * @base: NAND device instance > @@ -423,6 +437,7 @@ struct spinand_dirmap { > * the stack > * @manufacturer: SPI NAND manufacturer information > * @priv: manufacturer private data > + * @read_retries: the number of read retry modes supported > */ > struct spinand_device { > struct nand_device base; > @@ -449,8 +464,10 @@ struct spinand_device { > u8 *databuf; > u8 *oobbuf; > u8 *scratchbuf; > + const struct spinand_info *info; This looks like a leftover. > const struct spinand_manufacturer *manufacturer; > void *priv; > + int read_retries; Any reason to keep this variable signed? > }; > > /** Thanks, Miquèl ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nand: Add fixups for read retry @ 2024-10-01 9:40 ` Miquel Raynal 0 siblings, 0 replies; 23+ messages in thread From: Miquel Raynal @ 2024-10-01 9:40 UTC (permalink / raw) To: Cheng Ming Lin Cc: vigneshr, linux-mtd, linux-kernel, richard, alvinzhou, leoyu, Cheng Ming Lin Hi Cheng Ming, linchengming884@gmail.com wrote on Thu, 5 Sep 2024 13:53:32 +0800: > From: Cheng Ming Lin <chengminglin@mxic.com.tw> > > Add fixups for support read retry: > - Initialize the NAND device maximum retry mode. > - Set feature on Special Read for Data Recovery register. > > The Special Read for Data Recovery operation is enabled by Set Feature > function. > > There are 5 modes for the user to recover the lost data. > > Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw> > --- > drivers/mtd/nand/spi/macronix.c | 79 ++++++++++++++++++++++++++------- > include/linux/mtd/spinand.h | 17 +++++++ > 2 files changed, 81 insertions(+), 15 deletions(-) > > diff --git a/drivers/mtd/nand/spi/macronix.c b/drivers/mtd/nand/spi/macronix.c > index 3f9e9c572854..baca67ff1cd6 100644 > --- a/drivers/mtd/nand/spi/macronix.c > +++ b/drivers/mtd/nand/spi/macronix.c > @@ -9,6 +9,8 @@ > #include <linux/kernel.h> > #include <linux/mtd/spinand.h> > > +#define MACRONIX_NUM_READ_RETRY_MODES 6 You said 5 in the cover letter? > +#define MACRONIX_FEATURE_ADDR_READ_RETRY 0x70 Both definitions should probably come... > #define SPINAND_MFR_MACRONIX 0xC2 > #define MACRONIX_ECCSR_MASK 0x0F ...here > > @@ -100,6 +102,38 @@ static int mx35lf1ge4ab_ecc_get_status(struct spinand_device *spinand, > return -EINVAL; > } > > +/** > + * macronix_spinand_init_read_retry - Initialize read_retries > + * @spinand: SPI NAND device > + * > + * Return: the number of read retry modes > + */ > +static int macronix_spinand_init_read_retry(struct spinand_device *spinand) > +{ > + return MACRONIX_NUM_READ_RETRY_MODES; Does not sound very useful as a function? > +} > + > +/** > + * macronix_spinand_setup_read_retry - Set the retry mode > + * @spinand: SPI NAND device > + * @retry_mode: Specify which retry mode to set > + * > + * Return: 0 on success, -error otherwise , a negative error code otherwise. > + */ > +static int macronix_spinand_setup_read_retry(struct spinand_device *spinand, u8 retry_mode) > +{ > + struct spi_mem_op op = SPINAND_SET_FEATURE_OP(MACRONIX_FEATURE_ADDR_READ_RETRY, > + spinand->scratchbuf); > + > + *spinand->scratchbuf = retry_mode; > + return spi_mem_exec_op(spinand->spimem, &op); > +} > + > +static const struct spi_nand_fixups read_retry_fixups = { > + .init_read_retry = macronix_spinand_init_read_retry, > + .setup_read_retry = macronix_spinand_setup_read_retry, > +}; > + ... > @@ -325,7 +373,8 @@ static const struct spinand_info macronix_spinand_table[] = { > &update_cache_variants), > SPINAND_HAS_QE_BIT, > SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, > - mx35lf1ge4ab_ecc_get_status)), > + mx35lf1ge4ab_ecc_get_status), > + SPINAND_FIXUPS(&read_retry_fixups)), > SPINAND_INFO("MX35UF1GE4AC", > SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x92, 0x01), > NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1), I expect a patch targeting the core first, and then the changes in the Macronix driver. > diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h > index 5c19ead60499..e567d00a2805 100644 > --- a/include/linux/mtd/spinand.h > +++ b/include/linux/mtd/spinand.h > @@ -354,6 +354,7 @@ struct spinand_info { > } op_variants; > int (*select_target)(struct spinand_device *spinand, > unsigned int target); > + const struct spi_nand_fixups *fixups; > }; > > #define SPINAND_ID(__method, ...) \ > @@ -379,6 +380,9 @@ struct spinand_info { > #define SPINAND_SELECT_TARGET(__func) \ > .select_target = __func, > > +#define SPINAND_FIXUPS(__func) \ > + .fixups = __func, I don't like the fixups naming. It feels like something is inherently wrong. > + > #define SPINAND_INFO(__model, __id, __memorg, __eccreq, __op_variants, \ > __flags, ...) \ > { \ > @@ -398,6 +402,16 @@ struct spinand_dirmap { > struct spi_mem_dirmap_desc *rdesc_ecc; > }; > > +/** > + * struct spi_nand_fixups - SPI NAND fixup hooks > + * @init_read_retry: initialize spinand->read_retries > + * @setup_read_retry: set the retry mode > + */ > +struct spi_nand_fixups { > + int (*init_read_retry)(struct spinand_device *spinand); > + int (*setup_read_retry)(struct spinand_device *spinand, u8 retry_mode); > +}; > + > /** > * struct spinand_device - SPI NAND device instance > * @base: NAND device instance > @@ -423,6 +437,7 @@ struct spinand_dirmap { > * the stack > * @manufacturer: SPI NAND manufacturer information > * @priv: manufacturer private data > + * @read_retries: the number of read retry modes supported > */ > struct spinand_device { > struct nand_device base; > @@ -449,8 +464,10 @@ struct spinand_device { > u8 *databuf; > u8 *oobbuf; > u8 *scratchbuf; > + const struct spinand_info *info; This looks like a leftover. > const struct spinand_manufacturer *manufacturer; > void *priv; > + int read_retries; Any reason to keep this variable signed? > }; > > /** Thanks, Miquèl ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nand: Add fixups for read retry 2024-10-01 9:40 ` Miquel Raynal @ 2024-10-07 5:49 ` Cheng Ming Lin -1 siblings, 0 replies; 23+ messages in thread From: Cheng Ming Lin @ 2024-10-07 5:49 UTC (permalink / raw) To: Miquel Raynal Cc: vigneshr, linux-mtd, linux-kernel, richard, alvinzhou, leoyu, Cheng Ming Lin Hi Miquel, Miquel Raynal <miquel.raynal@bootlin.com> 於 2024年10月1日 週二 下午5:40寫道： > > Hi Cheng Ming, > > linchengming884@gmail.com wrote on Thu, 5 Sep 2024 13:53:32 +0800: > > > From: Cheng Ming Lin <chengminglin@mxic.com.tw> > > > > Add fixups for support read retry: > > - Initialize the NAND device maximum retry mode. > > - Set feature on Special Read for Data Recovery register. > > > > The Special Read for Data Recovery operation is enabled by Set Feature > > function. > > > > There are 5 modes for the user to recover the lost data. > > > > Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw> > > --- > > drivers/mtd/nand/spi/macronix.c | 79 ++++++++++++++++++++++++++------- > > include/linux/mtd/spinand.h | 17 +++++++ > > 2 files changed, 81 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/mtd/nand/spi/macronix.c b/drivers/mtd/nand/spi/macronix.c > > index 3f9e9c572854..baca67ff1cd6 100644 > > --- a/drivers/mtd/nand/spi/macronix.c > > +++ b/drivers/mtd/nand/spi/macronix.c > > @@ -9,6 +9,8 @@ > > #include <linux/kernel.h> > > #include <linux/mtd/spinand.h> > > > > +#define MACRONIX_NUM_READ_RETRY_MODES 6 > > You said 5 in the cover letter? Since the original mode is labeled as default in our datasheet, there are a total of six modes, including five additional modes numbered from Mode 1 to Mode 5. > > > +#define MACRONIX_FEATURE_ADDR_READ_RETRY 0x70 > > Both definitions should probably come... > > > #define SPINAND_MFR_MACRONIX 0xC2 > > #define MACRONIX_ECCSR_MASK 0x0F > > ...here Sure, thanks for your advice. > > > > > @@ -100,6 +102,38 @@ static int mx35lf1ge4ab_ecc_get_status(struct spinand_device *spinand, > > return -EINVAL; > > } > > > > +/** > > + * macronix_spinand_init_read_retry - Initialize read_retries > > + * @spinand: SPI NAND device > > + * > > + * Return: the number of read retry modes > > + */ > > +static int macronix_spinand_init_read_retry(struct spinand_device *spinand) > > +{ > > + return MACRONIX_NUM_READ_RETRY_MODES; > > Does not sound very useful as a function? > > > +} > > + > > +/** > > + * macronix_spinand_setup_read_retry - Set the retry mode > > + * @spinand: SPI NAND device > > + * @retry_mode: Specify which retry mode to set > > + * > > + * Return: 0 on success, -error otherwise > > , a negative error code otherwise. > > > + */ > > +static int macronix_spinand_setup_read_retry(struct spinand_device *spinand, u8 retry_mode) > > +{ > > + struct spi_mem_op op = SPINAND_SET_FEATURE_OP(MACRONIX_FEATURE_ADDR_READ_RETRY, > > + spinand->scratchbuf); > > + > > + *spinand->scratchbuf = retry_mode; > > + return spi_mem_exec_op(spinand->spimem, &op); > > +} > > + > > +static const struct spi_nand_fixups read_retry_fixups = { > > + .init_read_retry = macronix_spinand_init_read_retry, > > + .setup_read_retry = macronix_spinand_setup_read_retry, > > +}; > > + > > ... > > > @@ -325,7 +373,8 @@ static const struct spinand_info macronix_spinand_table[] = { > > &update_cache_variants), > > SPINAND_HAS_QE_BIT, > > SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, > > - mx35lf1ge4ab_ecc_get_status)), > > + mx35lf1ge4ab_ecc_get_status), > > + SPINAND_FIXUPS(&read_retry_fixups)), > > SPINAND_INFO("MX35UF1GE4AC", > > SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x92, 0x01), > > NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1), > > I expect a patch targeting the core first, and then the changes in the > Macronix driver. Got it, so do you prefer that we switch to using flags instead? > > > diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h > > index 5c19ead60499..e567d00a2805 100644 > > --- a/include/linux/mtd/spinand.h > > +++ b/include/linux/mtd/spinand.h > > @@ -354,6 +354,7 @@ struct spinand_info { > > } op_variants; > > int (*select_target)(struct spinand_device *spinand, > > unsigned int target); > > + const struct spi_nand_fixups *fixups; > > }; > > > > #define SPINAND_ID(__method, ...) \ > > @@ -379,6 +380,9 @@ struct spinand_info { > > #define SPINAND_SELECT_TARGET(__func) \ > > .select_target = __func, > > > > +#define SPINAND_FIXUPS(__func) \ > > + .fixups = __func, > > I don't like the fixups naming. It feels like something is inherently > wrong. > > > + > > #define SPINAND_INFO(__model, __id, __memorg, __eccreq, __op_variants, \ > > __flags, ...) \ > > { \ > > @@ -398,6 +402,16 @@ struct spinand_dirmap { > > struct spi_mem_dirmap_desc *rdesc_ecc; > > }; > > > > +/** > > + * struct spi_nand_fixups - SPI NAND fixup hooks > > + * @init_read_retry: initialize spinand->read_retries > > + * @setup_read_retry: set the retry mode > > + */ > > +struct spi_nand_fixups { > > + int (*init_read_retry)(struct spinand_device *spinand); > > + int (*setup_read_retry)(struct spinand_device *spinand, u8 retry_mode); > > +}; > > + > > /** > > * struct spinand_device - SPI NAND device instance > > * @base: NAND device instance > > @@ -423,6 +437,7 @@ struct spinand_dirmap { > > * the stack > > * @manufacturer: SPI NAND manufacturer information > > * @priv: manufacturer private data > > + * @read_retries: the number of read retry modes supported > > */ > > struct spinand_device { > > struct nand_device base; > > @@ -449,8 +464,10 @@ struct spinand_device { > > u8 *databuf; > > u8 *oobbuf; > > u8 *scratchbuf; > > + const struct spinand_info *info; > > This looks like a leftover. > > > const struct spinand_manufacturer *manufacturer; > > void *priv; > > + int read_retries; > > Any reason to keep this variable signed? No, we can simply change from int to u8. > > > }; > > > > /** > > > Thanks, > Miquèl Thanks, Cheng Ming Lin ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nand: Add fixups for read retry @ 2024-10-07 5:49 ` Cheng Ming Lin 0 siblings, 0 replies; 23+ messages in thread From: Cheng Ming Lin @ 2024-10-07 5:49 UTC (permalink / raw) To: Miquel Raynal Cc: vigneshr, linux-mtd, linux-kernel, richard, alvinzhou, leoyu, Cheng Ming Lin Hi Miquel, Miquel Raynal <miquel.raynal@bootlin.com> 於 2024年10月1日 週二 下午5:40寫道： > > Hi Cheng Ming, > > linchengming884@gmail.com wrote on Thu, 5 Sep 2024 13:53:32 +0800: > > > From: Cheng Ming Lin <chengminglin@mxic.com.tw> > > > > Add fixups for support read retry: > > - Initialize the NAND device maximum retry mode. > > - Set feature on Special Read for Data Recovery register. > > > > The Special Read for Data Recovery operation is enabled by Set Feature > > function. > > > > There are 5 modes for the user to recover the lost data. > > > > Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw> > > --- > > drivers/mtd/nand/spi/macronix.c | 79 ++++++++++++++++++++++++++------- > > include/linux/mtd/spinand.h | 17 +++++++ > > 2 files changed, 81 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/mtd/nand/spi/macronix.c b/drivers/mtd/nand/spi/macronix.c > > index 3f9e9c572854..baca67ff1cd6 100644 > > --- a/drivers/mtd/nand/spi/macronix.c > > +++ b/drivers/mtd/nand/spi/macronix.c > > @@ -9,6 +9,8 @@ > > #include <linux/kernel.h> > > #include <linux/mtd/spinand.h> > > > > +#define MACRONIX_NUM_READ_RETRY_MODES 6 > > You said 5 in the cover letter? Since the original mode is labeled as default in our datasheet, there are a total of six modes, including five additional modes numbered from Mode 1 to Mode 5. > > > +#define MACRONIX_FEATURE_ADDR_READ_RETRY 0x70 > > Both definitions should probably come... > > > #define SPINAND_MFR_MACRONIX 0xC2 > > #define MACRONIX_ECCSR_MASK 0x0F > > ...here Sure, thanks for your advice. > > > > > @@ -100,6 +102,38 @@ static int mx35lf1ge4ab_ecc_get_status(struct spinand_device *spinand, > > return -EINVAL; > > } > > > > +/** > > + * macronix_spinand_init_read_retry - Initialize read_retries > > + * @spinand: SPI NAND device > > + * > > + * Return: the number of read retry modes > > + */ > > +static int macronix_spinand_init_read_retry(struct spinand_device *spinand) > > +{ > > + return MACRONIX_NUM_READ_RETRY_MODES; > > Does not sound very useful as a function? > > > +} > > + > > +/** > > + * macronix_spinand_setup_read_retry - Set the retry mode > > + * @spinand: SPI NAND device > > + * @retry_mode: Specify which retry mode to set > > + * > > + * Return: 0 on success, -error otherwise > > , a negative error code otherwise. > > > + */ > > +static int macronix_spinand_setup_read_retry(struct spinand_device *spinand, u8 retry_mode) > > +{ > > + struct spi_mem_op op = SPINAND_SET_FEATURE_OP(MACRONIX_FEATURE_ADDR_READ_RETRY, > > + spinand->scratchbuf); > > + > > + *spinand->scratchbuf = retry_mode; > > + return spi_mem_exec_op(spinand->spimem, &op); > > +} > > + > > +static const struct spi_nand_fixups read_retry_fixups = { > > + .init_read_retry = macronix_spinand_init_read_retry, > > + .setup_read_retry = macronix_spinand_setup_read_retry, > > +}; > > + > > ... > > > @@ -325,7 +373,8 @@ static const struct spinand_info macronix_spinand_table[] = { > > &update_cache_variants), > > SPINAND_HAS_QE_BIT, > > SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, > > - mx35lf1ge4ab_ecc_get_status)), > > + mx35lf1ge4ab_ecc_get_status), > > + SPINAND_FIXUPS(&read_retry_fixups)), > > SPINAND_INFO("MX35UF1GE4AC", > > SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x92, 0x01), > > NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1), > > I expect a patch targeting the core first, and then the changes in the > Macronix driver. Got it, so do you prefer that we switch to using flags instead? > > > diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h > > index 5c19ead60499..e567d00a2805 100644 > > --- a/include/linux/mtd/spinand.h > > +++ b/include/linux/mtd/spinand.h > > @@ -354,6 +354,7 @@ struct spinand_info { > > } op_variants; > > int (*select_target)(struct spinand_device *spinand, > > unsigned int target); > > + const struct spi_nand_fixups *fixups; > > }; > > > > #define SPINAND_ID(__method, ...) \ > > @@ -379,6 +380,9 @@ struct spinand_info { > > #define SPINAND_SELECT_TARGET(__func) \ > > .select_target = __func, > > > > +#define SPINAND_FIXUPS(__func) \ > > + .fixups = __func, > > I don't like the fixups naming. It feels like something is inherently > wrong. > > > + > > #define SPINAND_INFO(__model, __id, __memorg, __eccreq, __op_variants, \ > > __flags, ...) \ > > { \ > > @@ -398,6 +402,16 @@ struct spinand_dirmap { > > struct spi_mem_dirmap_desc *rdesc_ecc; > > }; > > > > +/** > > + * struct spi_nand_fixups - SPI NAND fixup hooks > > + * @init_read_retry: initialize spinand->read_retries > > + * @setup_read_retry: set the retry mode > > + */ > > +struct spi_nand_fixups { > > + int (*init_read_retry)(struct spinand_device *spinand); > > + int (*setup_read_retry)(struct spinand_device *spinand, u8 retry_mode); > > +}; > > + > > /** > > * struct spinand_device - SPI NAND device instance > > * @base: NAND device instance > > @@ -423,6 +437,7 @@ struct spinand_dirmap { > > * the stack > > * @manufacturer: SPI NAND manufacturer information > > * @priv: manufacturer private data > > + * @read_retries: the number of read retry modes supported > > */ > > struct spinand_device { > > struct nand_device base; > > @@ -449,8 +464,10 @@ struct spinand_device { > > u8 *databuf; > > u8 *oobbuf; > > u8 *scratchbuf; > > + const struct spinand_info *info; > > This looks like a leftover. > > > const struct spinand_manufacturer *manufacturer; > > void *priv; > > + int read_retries; > > Any reason to keep this variable signed? No, we can simply change from int to u8. > > > }; > > > > /** > > > Thanks, > Miquèl Thanks, Cheng Ming Lin ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nand: Add fixups for read retry 2024-10-07 5:49 ` Cheng Ming Lin @ 2024-10-07 8:33 ` Miquel Raynal -1 siblings, 0 replies; 23+ messages in thread From: Miquel Raynal @ 2024-10-07 8:33 UTC (permalink / raw) To: Cheng Ming Lin Cc: vigneshr, linux-mtd, linux-kernel, richard, alvinzhou, leoyu, Cheng Ming Lin Hi Cheng Ming, > > > @@ -325,7 +373,8 @@ static const struct spinand_info macronix_spinand_table[] = { > > > &update_cache_variants), > > > SPINAND_HAS_QE_BIT, > > > SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, > > > - mx35lf1ge4ab_ecc_get_status)), > > > + mx35lf1ge4ab_ecc_get_status), > > > + SPINAND_FIXUPS(&read_retry_fixups)), > > > SPINAND_INFO("MX35UF1GE4AC", > > > SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x92, 0x01), > > > NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1), > > > > I expect a patch targeting the core first, and then the changes in the > > Macronix driver. > > Got it, so do you prefer that we switch to using flags instead? Not necessarily, did I? ... > > > const struct spinand_manufacturer *manufacturer; > > > void *priv; > > > + int read_retries; > > > > Any reason to keep this variable signed? > > No, we can simply change from int to u8. Just unsigned int is fine. Thanks, Miquèl ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nand: Add fixups for read retry @ 2024-10-07 8:33 ` Miquel Raynal 0 siblings, 0 replies; 23+ messages in thread From: Miquel Raynal @ 2024-10-07 8:33 UTC (permalink / raw) To: Cheng Ming Lin Cc: vigneshr, linux-mtd, linux-kernel, richard, alvinzhou, leoyu, Cheng Ming Lin Hi Cheng Ming, > > > @@ -325,7 +373,8 @@ static const struct spinand_info macronix_spinand_table[] = { > > > &update_cache_variants), > > > SPINAND_HAS_QE_BIT, > > > SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, > > > - mx35lf1ge4ab_ecc_get_status)), > > > + mx35lf1ge4ab_ecc_get_status), > > > + SPINAND_FIXUPS(&read_retry_fixups)), > > > SPINAND_INFO("MX35UF1GE4AC", > > > SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x92, 0x01), > > > NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1), > > > > I expect a patch targeting the core first, and then the changes in the > > Macronix driver. > > Got it, so do you prefer that we switch to using flags instead? Not necessarily, did I? ... > > > const struct spinand_manufacturer *manufacturer; > > > void *priv; > > > + int read_retries; > > > > Any reason to keep this variable signed? > > No, we can simply change from int to u8. Just unsigned int is fine. Thanks, Miquèl ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nand: Add fixups for read retry 2024-10-07 8:33 ` Miquel Raynal @ 2024-10-08 6:25 ` Cheng Ming Lin -1 siblings, 0 replies; 23+ messages in thread From: Cheng Ming Lin @ 2024-10-08 6:25 UTC (permalink / raw) To: Miquel Raynal Cc: vigneshr, linux-mtd, linux-kernel, richard, alvinzhou, leoyu, Cheng Ming Lin Hi Miquel, Miquel Raynal <miquel.raynal@bootlin.com> 於 2024年10月7日 週一 下午4:33寫道： > > Hi Cheng Ming, > > > > > @@ -325,7 +373,8 @@ static const struct spinand_info macronix_spinand_table[] = { > > > > &update_cache_variants), > > > > SPINAND_HAS_QE_BIT, > > > > SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, > > > > - mx35lf1ge4ab_ecc_get_status)), > > > > + mx35lf1ge4ab_ecc_get_status), > > > > + SPINAND_FIXUPS(&read_retry_fixups)), > > > > SPINAND_INFO("MX35UF1GE4AC", > > > > SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x92, 0x01), > > > > NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1), > > > > > > I expect a patch targeting the core first, and then the changes in the > > > Macronix driver. > > > > Got it, so do you prefer that we switch to using flags instead? > > Not necessarily, did I? > > ... > Using a flag instead of fixups allows this patch to target the core first, and reduces changes in the Macronix driver. > > > > const struct spinand_manufacturer *manufacturer; > > > > void *priv; > > > > + int read_retries; > > > > > > Any reason to keep this variable signed? > > > > No, we can simply change from int to u8. > > Just unsigned int is fine. > Sure, thanks! > Thanks, > Miquèl Thanks, Cheng Ming Lin ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nand: Add fixups for read retry @ 2024-10-08 6:25 ` Cheng Ming Lin 0 siblings, 0 replies; 23+ messages in thread From: Cheng Ming Lin @ 2024-10-08 6:25 UTC (permalink / raw) To: Miquel Raynal Cc: vigneshr, linux-mtd, linux-kernel, richard, alvinzhou, leoyu, Cheng Ming Lin Hi Miquel, Miquel Raynal <miquel.raynal@bootlin.com> 於 2024年10月7日 週一 下午4:33寫道： > > Hi Cheng Ming, > > > > > @@ -325,7 +373,8 @@ static const struct spinand_info macronix_spinand_table[] = { > > > > &update_cache_variants), > > > > SPINAND_HAS_QE_BIT, > > > > SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, > > > > - mx35lf1ge4ab_ecc_get_status)), > > > > + mx35lf1ge4ab_ecc_get_status), > > > > + SPINAND_FIXUPS(&read_retry_fixups)), > > > > SPINAND_INFO("MX35UF1GE4AC", > > > > SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x92, 0x01), > > > > NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1), > > > > > > I expect a patch targeting the core first, and then the changes in the > > > Macronix driver. > > > > Got it, so do you prefer that we switch to using flags instead? > > Not necessarily, did I? > > ... > Using a flag instead of fixups allows this patch to target the core first, and reduces changes in the Macronix driver. > > > > const struct spinand_manufacturer *manufacturer; > > > > void *priv; > > > > + int read_retries; > > > > > > Any reason to keep this variable signed? > > > > No, we can simply change from int to u8. > > Just unsigned int is fine. > Sure, thanks! > Thanks, > Miquèl Thanks, Cheng Ming Lin ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nand: Add fixups for read retry 2024-10-08 6:25 ` Cheng Ming Lin @ 2024-10-08 8:55 ` Miquel Raynal -1 siblings, 0 replies; 23+ messages in thread From: Miquel Raynal @ 2024-10-08 8:55 UTC (permalink / raw) To: Cheng Ming Lin Cc: vigneshr, linux-mtd, linux-kernel, richard, alvinzhou, leoyu, Cheng Ming Lin Hi, linchengming884@gmail.com wrote on Tue, 8 Oct 2024 14:25:25 +0800: > Hi Miquel, > > Miquel Raynal <miquel.raynal@bootlin.com> 於 2024年10月7日 週一 下午4:33寫道： > > > > Hi Cheng Ming, > > > > > > > @@ -325,7 +373,8 @@ static const struct spinand_info macronix_spinand_table[] = { > > > > > &update_cache_variants), > > > > > SPINAND_HAS_QE_BIT, > > > > > SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, > > > > > - mx35lf1ge4ab_ecc_get_status)), > > > > > + mx35lf1ge4ab_ecc_get_status), > > > > > + SPINAND_FIXUPS(&read_retry_fixups)), > > > > > SPINAND_INFO("MX35UF1GE4AC", > > > > > SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x92, 0x01), > > > > > NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1), > > > > > > > > I expect a patch targeting the core first, and then the changes in the > > > > Macronix driver. > > > > > > Got it, so do you prefer that we switch to using flags instead? > > > > Not necessarily, did I? > > > > ... > > > > Using a flag instead of fixups allows this patch to target the core first, > and reduces changes in the Macronix driver. Propose what ever you think is best. You can also look at how it is done in raw NAND. But always include the core changes first, please. It is not related to how you implement it. > > > > > > const struct spinand_manufacturer *manufacturer; > > > > > void *priv; > > > > > + int read_retries; > > > > > > > > Any reason to keep this variable signed? > > > > > > No, we can simply change from int to u8. > > > > Just unsigned int is fine. > > > > Sure, thanks! > > > Thanks, > > Miquèl > > Thanks, > Cheng Ming Lin Thanks, Miquèl ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nand: Add fixups for read retry @ 2024-10-08 8:55 ` Miquel Raynal 0 siblings, 0 replies; 23+ messages in thread From: Miquel Raynal @ 2024-10-08 8:55 UTC (permalink / raw) To: Cheng Ming Lin Cc: vigneshr, linux-mtd, linux-kernel, richard, alvinzhou, leoyu, Cheng Ming Lin Hi, linchengming884@gmail.com wrote on Tue, 8 Oct 2024 14:25:25 +0800: > Hi Miquel, > > Miquel Raynal <miquel.raynal@bootlin.com> 於 2024年10月7日 週一 下午4:33寫道： > > > > Hi Cheng Ming, > > > > > > > @@ -325,7 +373,8 @@ static const struct spinand_info macronix_spinand_table[] = { > > > > > &update_cache_variants), > > > > > SPINAND_HAS_QE_BIT, > > > > > SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, > > > > > - mx35lf1ge4ab_ecc_get_status)), > > > > > + mx35lf1ge4ab_ecc_get_status), > > > > > + SPINAND_FIXUPS(&read_retry_fixups)), > > > > > SPINAND_INFO("MX35UF1GE4AC", > > > > > SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x92, 0x01), > > > > > NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1), > > > > > > > > I expect a patch targeting the core first, and then the changes in the > > > > Macronix driver. > > > > > > Got it, so do you prefer that we switch to using flags instead? > > > > Not necessarily, did I? > > > > ... > > > > Using a flag instead of fixups allows this patch to target the core first, > and reduces changes in the Macronix driver. Propose what ever you think is best. You can also look at how it is done in raw NAND. But always include the core changes first, please. It is not related to how you implement it. > > > > > > const struct spinand_manufacturer *manufacturer; > > > > > void *priv; > > > > > + int read_retries; > > > > > > > > Any reason to keep this variable signed? > > > > > > No, we can simply change from int to u8. > > > > Just unsigned int is fine. > > > > Sure, thanks! > > > Thanks, > > Miquèl > > Thanks, > Cheng Ming Lin Thanks, Miquèl ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nand: Add fixups for read retry 2024-10-08 8:55 ` Miquel Raynal @ 2024-10-08 9:19 ` Cheng Ming Lin -1 siblings, 0 replies; 23+ messages in thread From: Cheng Ming Lin @ 2024-10-08 9:19 UTC (permalink / raw) To: Miquel Raynal Cc: vigneshr, linux-mtd, linux-kernel, richard, alvinzhou, leoyu, Cheng Ming Lin Hi Miquel, Miquel Raynal <miquel.raynal@bootlin.com> 於 2024年10月8日 週二 下午4:55寫道： > > Hi, > > linchengming884@gmail.com wrote on Tue, 8 Oct 2024 14:25:25 +0800: > > > Hi Miquel, > > > > Miquel Raynal <miquel.raynal@bootlin.com> 於 2024年10月7日 週一 下午4:33寫道： > > > > > > Hi Cheng Ming, > > > > > > > > > @@ -325,7 +373,8 @@ static const struct spinand_info macronix_spinand_table[] = { > > > > > > &update_cache_variants), > > > > > > SPINAND_HAS_QE_BIT, > > > > > > SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, > > > > > > - mx35lf1ge4ab_ecc_get_status)), > > > > > > + mx35lf1ge4ab_ecc_get_status), > > > > > > + SPINAND_FIXUPS(&read_retry_fixups)), > > > > > > SPINAND_INFO("MX35UF1GE4AC", > > > > > > SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x92, 0x01), > > > > > > NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1), > > > > > > > > > > I expect a patch targeting the core first, and then the changes in the > > > > > Macronix driver. > > > > > > > > Got it, so do you prefer that we switch to using flags instead? > > > > > > Not necessarily, did I? > > > > > > ... > > > > > > > Using a flag instead of fixups allows this patch to target the core first, > > and reduces changes in the Macronix driver. > > Propose what ever you think is best. You can also look at how it is > done in raw NAND. But always include the core changes first, please. > It is not related to how you implement it. > Thank you so much for your suggestion. I will ensure the core changes are addressed first, as you mentioned. Also, I did refer to how it's done in raw NAND when working on this patch. I really appreciate your guidance and will definitely keep your words in mind. > > > > > > > > const struct spinand_manufacturer *manufacturer; > > > > > > void *priv; > > > > > > + int read_retries; > > > > > > > > > > Any reason to keep this variable signed? > > > > > > > > No, we can simply change from int to u8. > > > > > > Just unsigned int is fine. > > > > > > > Sure, thanks! > > > > > Thanks, > > > Miquèl > > > > Thanks, > > Cheng Ming Lin > > > Thanks, > Miquèl Thanks, Cheng Ming Lin ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nand: Add fixups for read retry @ 2024-10-08 9:19 ` Cheng Ming Lin 0 siblings, 0 replies; 23+ messages in thread From: Cheng Ming Lin @ 2024-10-08 9:19 UTC (permalink / raw) To: Miquel Raynal Cc: vigneshr, linux-mtd, linux-kernel, richard, alvinzhou, leoyu, Cheng Ming Lin Hi Miquel, Miquel Raynal <miquel.raynal@bootlin.com> 於 2024年10月8日 週二 下午4:55寫道： > > Hi, > > linchengming884@gmail.com wrote on Tue, 8 Oct 2024 14:25:25 +0800: > > > Hi Miquel, > > > > Miquel Raynal <miquel.raynal@bootlin.com> 於 2024年10月7日 週一 下午4:33寫道： > > > > > > Hi Cheng Ming, > > > > > > > > > @@ -325,7 +373,8 @@ static const struct spinand_info macronix_spinand_table[] = { > > > > > > &update_cache_variants), > > > > > > SPINAND_HAS_QE_BIT, > > > > > > SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, > > > > > > - mx35lf1ge4ab_ecc_get_status)), > > > > > > + mx35lf1ge4ab_ecc_get_status), > > > > > > + SPINAND_FIXUPS(&read_retry_fixups)), > > > > > > SPINAND_INFO("MX35UF1GE4AC", > > > > > > SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x92, 0x01), > > > > > > NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1), > > > > > > > > > > I expect a patch targeting the core first, and then the changes in the > > > > > Macronix driver. > > > > > > > > Got it, so do you prefer that we switch to using flags instead? > > > > > > Not necessarily, did I? > > > > > > ... > > > > > > > Using a flag instead of fixups allows this patch to target the core first, > > and reduces changes in the Macronix driver. > > Propose what ever you think is best. You can also look at how it is > done in raw NAND. But always include the core changes first, please. > It is not related to how you implement it. > Thank you so much for your suggestion. I will ensure the core changes are addressed first, as you mentioned. Also, I did refer to how it's done in raw NAND when working on this patch. I really appreciate your guidance and will definitely keep your words in mind. > > > > > > > > const struct spinand_manufacturer *manufacturer; > > > > > > void *priv; > > > > > > + int read_retries; > > > > > > > > > > Any reason to keep this variable signed? > > > > > > > > No, we can simply change from int to u8. > > > > > > Just unsigned int is fine. > > > > > > > Sure, thanks! > > > > > Thanks, > > > Miquèl > > > > Thanks, > > Cheng Ming Lin > > > Thanks, > Miquèl Thanks, Cheng Ming Lin ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/2] mtd: spi-nand: Add read retry support 2024-09-05 5:53 ` Cheng Ming Lin @ 2024-09-05 5:53 ` Cheng Ming Lin -1 siblings, 0 replies; 23+ messages in thread From: Cheng Ming Lin @ 2024-09-05 5:53 UTC (permalink / raw) To: miquel.raynal, vigneshr, linux-mtd, linux-kernel Cc: richard, alvinzhou, leoyu, Cheng Ming Lin From: Cheng Ming Lin <chengminglin@mxic.com.tw> When the host ECC fails to correct the data error of NAND device, there's a special read for data recovery method which host setups for the next read retry mode and may recover the lost data by host ECC again. Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw> --- drivers/mtd/nand/spi/core.c | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c index e0b6715e5dfe..2f21ea926132 100644 --- a/drivers/mtd/nand/spi/core.c +++ b/drivers/mtd/nand/spi/core.c @@ -640,6 +640,7 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from, struct nand_io_iter iter; bool disable_ecc = false; bool ecc_failed = false; + u8 retry_mode = 0; int ret = 0; if (ops->mode == MTD_OPS_RAW || !spinand->eccinfo.ooblayout) @@ -657,20 +658,45 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from, if (ret) break; +read_retry: ret = spinand_read_page(spinand, &iter.req); if (ret < 0 && ret != -EBADMSG) break; - if (ret == -EBADMSG) + if (ret == -EBADMSG && spinand->info->fixups) { + if (spinand->read_retries && ((retry_mode + 1) < spinand->read_retries)) { + retry_mode++; + ret = spinand->info->fixups->setup_read_retry(spinand, retry_mode); + if (ret < 0) + break; + + /* Reset ecc_stats; retry */ + mtd->ecc_stats = old_stats; + goto read_retry; + } else { + /* No more retry modes; real failure */ + ecc_failed = true; + } + } else if (ret == -EBADMSG) { ecc_failed = true; - else + } else { max_bitflips = max_t(unsigned int, max_bitflips, ret); + } ret = 0; ops->retlen += iter.req.datalen; ops->oobretlen += iter.req.ooblen; + + /* Reset to retry mode 0*/ + if (retry_mode) { + ret = spinand->info->fixups->setup_read_retry(spinand, 0); + if (ret < 0) + break; + retry_mode = 0; + } } + if (ops->stats) { ops->stats->uncorrectable_errors += mtd->ecc_stats.failed - old_stats.failed; @@ -1095,6 +1121,9 @@ int spinand_match_and_init(struct spinand_device *spinand, spinand->flags = table[i].flags; spinand->id.len = 1 + table[i].devid.len; spinand->select_target = table[i].select_target; + spinand->info = info; + if (spinand->info->fixups && spinand->info->fixups->init_read_retry) + spinand->read_retries = spinand->info->fixups->init_read_retry(spinand); op = spinand_select_op_variant(spinand, info->op_variants.read_cache); -- 2.25.1 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 2/2] mtd: spi-nand: Add read retry support @ 2024-09-05 5:53 ` Cheng Ming Lin 0 siblings, 0 replies; 23+ messages in thread From: Cheng Ming Lin @ 2024-09-05 5:53 UTC (permalink / raw) To: miquel.raynal, vigneshr, linux-mtd, linux-kernel Cc: richard, alvinzhou, leoyu, Cheng Ming Lin From: Cheng Ming Lin <chengminglin@mxic.com.tw> When the host ECC fails to correct the data error of NAND device, there's a special read for data recovery method which host setups for the next read retry mode and may recover the lost data by host ECC again. Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw> --- drivers/mtd/nand/spi/core.c | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c index e0b6715e5dfe..2f21ea926132 100644 --- a/drivers/mtd/nand/spi/core.c +++ b/drivers/mtd/nand/spi/core.c @@ -640,6 +640,7 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from, struct nand_io_iter iter; bool disable_ecc = false; bool ecc_failed = false; + u8 retry_mode = 0; int ret = 0; if (ops->mode == MTD_OPS_RAW || !spinand->eccinfo.ooblayout) @@ -657,20 +658,45 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from, if (ret) break; +read_retry: ret = spinand_read_page(spinand, &iter.req); if (ret < 0 && ret != -EBADMSG) break; - if (ret == -EBADMSG) + if (ret == -EBADMSG && spinand->info->fixups) { + if (spinand->read_retries && ((retry_mode + 1) < spinand->read_retries)) { + retry_mode++; + ret = spinand->info->fixups->setup_read_retry(spinand, retry_mode); + if (ret < 0) + break; + + /* Reset ecc_stats; retry */ + mtd->ecc_stats = old_stats; + goto read_retry; + } else { + /* No more retry modes; real failure */ + ecc_failed = true; + } + } else if (ret == -EBADMSG) { ecc_failed = true; - else + } else { max_bitflips = max_t(unsigned int, max_bitflips, ret); + } ret = 0; ops->retlen += iter.req.datalen; ops->oobretlen += iter.req.ooblen; + + /* Reset to retry mode 0*/ + if (retry_mode) { + ret = spinand->info->fixups->setup_read_retry(spinand, 0); + if (ret < 0) + break; + retry_mode = 0; + } } + if (ops->stats) { ops->stats->uncorrectable_errors += mtd->ecc_stats.failed - old_stats.failed; @@ -1095,6 +1121,9 @@ int spinand_match_and_init(struct spinand_device *spinand, spinand->flags = table[i].flags; spinand->id.len = 1 + table[i].devid.len; spinand->select_target = table[i].select_target; + spinand->info = info; + if (spinand->info->fixups && spinand->info->fixups->init_read_retry) + spinand->read_retries = spinand->info->fixups->init_read_retry(spinand); op = spinand_select_op_variant(spinand, info->op_variants.read_cache); -- 2.25.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 2/2] mtd: spi-nand: Add read retry support 2024-09-05 5:53 ` Cheng Ming Lin @ 2024-10-01 10:17 ` Miquel Raynal -1 siblings, 0 replies; 23+ messages in thread From: Miquel Raynal @ 2024-10-01 10:17 UTC (permalink / raw) To: Cheng Ming Lin Cc: vigneshr, linux-mtd, linux-kernel, richard, alvinzhou, leoyu, Cheng Ming Lin Hi Cheng Ming, linchengming884@gmail.com wrote on Thu, 5 Sep 2024 13:53:33 +0800: > From: Cheng Ming Lin <chengminglin@mxic.com.tw> > > When the host ECC fails to correct the data error of NAND device, > there's a special read for data recovery method which host setups > for the next read retry mode and may recover the lost data by host > ECC again. > > Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw> > --- > drivers/mtd/nand/spi/core.c | 33 +++++++++++++++++++++++++++++++-- > 1 file changed, 31 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c > index e0b6715e5dfe..2f21ea926132 100644 > --- a/drivers/mtd/nand/spi/core.c > +++ b/drivers/mtd/nand/spi/core.c > @@ -640,6 +640,7 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from, > struct nand_io_iter iter; > bool disable_ecc = false; > bool ecc_failed = false; > + u8 retry_mode = 0; > int ret = 0; > > if (ops->mode == MTD_OPS_RAW || !spinand->eccinfo.ooblayout) > @@ -657,20 +658,45 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from, This will no longer apply with continuous support added in. However please make this only in the non continuous path. I don't think it can fly in the other. > if (ret) > break; > > +read_retry: > ret = spinand_read_page(spinand, &iter.req); > if (ret < 0 && ret != -EBADMSG) > break; > > - if (ret == -EBADMSG) > + if (ret == -EBADMSG && spinand->info->fixups) { > + if (spinand->read_retries && ((retry_mode + 1) < spinand->read_retries)) { ++retry_mode? > + retry_mode++; So this can be dropped. > + ret = spinand->info->fixups->setup_read_retry(spinand, retry_mode); > + if (ret < 0) > + break; No, you need to set ecc_failed here. > + > + /* Reset ecc_stats; retry */ > + mtd->ecc_stats = old_stats; > + goto read_retry; > + } else { > + /* No more retry modes; real failure */ > + ecc_failed = true; > + } > + } else if (ret == -EBADMSG) { > ecc_failed = true; > - else > + } else { > max_bitflips = max_t(unsigned int, max_bitflips, ret); > + } > > ret = 0; > ops->retlen += iter.req.datalen; > ops->oobretlen += iter.req.ooblen; > + > + /* Reset to retry mode 0*/ > + if (retry_mode) { retry_mode = 0; > + ret = spinand->info->fixups->setup_read_retry(spinand, 0); retry_mode); > + if (ret < 0) > + break; this if clause is useless. > + retry_mode = 0; And then this can be dropped from here. > + } > } > > + Spurious line > if (ops->stats) { > ops->stats->uncorrectable_errors += > mtd->ecc_stats.failed - old_stats.failed; > @@ -1095,6 +1121,9 @@ int spinand_match_and_init(struct spinand_device *spinand, > spinand->flags = table[i].flags; > spinand->id.len = 1 + table[i].devid.len; > spinand->select_target = table[i].select_target; > + spinand->info = info; > + if (spinand->info->fixups && spinand->info->fixups->init_read_retry) > + spinand->read_retries = spinand->info->fixups->init_read_retry(spinand); Now I get you init. Ok, fine. > > op = spinand_select_op_variant(spinand, > info->op_variants.read_cache); Thanks, Miquèl ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/2] mtd: spi-nand: Add read retry support @ 2024-10-01 10:17 ` Miquel Raynal 0 siblings, 0 replies; 23+ messages in thread From: Miquel Raynal @ 2024-10-01 10:17 UTC (permalink / raw) To: Cheng Ming Lin Cc: vigneshr, linux-mtd, linux-kernel, richard, alvinzhou, leoyu, Cheng Ming Lin Hi Cheng Ming, linchengming884@gmail.com wrote on Thu, 5 Sep 2024 13:53:33 +0800: > From: Cheng Ming Lin <chengminglin@mxic.com.tw> > > When the host ECC fails to correct the data error of NAND device, > there's a special read for data recovery method which host setups > for the next read retry mode and may recover the lost data by host > ECC again. > > Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw> > --- > drivers/mtd/nand/spi/core.c | 33 +++++++++++++++++++++++++++++++-- > 1 file changed, 31 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c > index e0b6715e5dfe..2f21ea926132 100644 > --- a/drivers/mtd/nand/spi/core.c > +++ b/drivers/mtd/nand/spi/core.c > @@ -640,6 +640,7 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from, > struct nand_io_iter iter; > bool disable_ecc = false; > bool ecc_failed = false; > + u8 retry_mode = 0; > int ret = 0; > > if (ops->mode == MTD_OPS_RAW || !spinand->eccinfo.ooblayout) > @@ -657,20 +658,45 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from, This will no longer apply with continuous support added in. However please make this only in the non continuous path. I don't think it can fly in the other. > if (ret) > break; > > +read_retry: > ret = spinand_read_page(spinand, &iter.req); > if (ret < 0 && ret != -EBADMSG) > break; > > - if (ret == -EBADMSG) > + if (ret == -EBADMSG && spinand->info->fixups) { > + if (spinand->read_retries && ((retry_mode + 1) < spinand->read_retries)) { ++retry_mode? > + retry_mode++; So this can be dropped. > + ret = spinand->info->fixups->setup_read_retry(spinand, retry_mode); > + if (ret < 0) > + break; No, you need to set ecc_failed here. > + > + /* Reset ecc_stats; retry */ > + mtd->ecc_stats = old_stats; > + goto read_retry; > + } else { > + /* No more retry modes; real failure */ > + ecc_failed = true; > + } > + } else if (ret == -EBADMSG) { > ecc_failed = true; > - else > + } else { > max_bitflips = max_t(unsigned int, max_bitflips, ret); > + } > > ret = 0; > ops->retlen += iter.req.datalen; > ops->oobretlen += iter.req.ooblen; > + > + /* Reset to retry mode 0*/ > + if (retry_mode) { retry_mode = 0; > + ret = spinand->info->fixups->setup_read_retry(spinand, 0); retry_mode); > + if (ret < 0) > + break; this if clause is useless. > + retry_mode = 0; And then this can be dropped from here. > + } > } > > + Spurious line > if (ops->stats) { > ops->stats->uncorrectable_errors += > mtd->ecc_stats.failed - old_stats.failed; > @@ -1095,6 +1121,9 @@ int spinand_match_and_init(struct spinand_device *spinand, > spinand->flags = table[i].flags; > spinand->id.len = 1 + table[i].devid.len; > spinand->select_target = table[i].select_target; > + spinand->info = info; > + if (spinand->info->fixups && spinand->info->fixups->init_read_retry) > + spinand->read_retries = spinand->info->fixups->init_read_retry(spinand); Now I get you init. Ok, fine. > > op = spinand_select_op_variant(spinand, > info->op_variants.read_cache); Thanks, Miquèl ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/2] mtd: spi-nand: Add read retry support 2024-10-01 10:17 ` Miquel Raynal @ 2024-10-07 5:53 ` Cheng Ming Lin -1 siblings, 0 replies; 23+ messages in thread From: Cheng Ming Lin @ 2024-10-07 5:53 UTC (permalink / raw) To: Miquel Raynal Cc: vigneshr, linux-mtd, linux-kernel, richard, alvinzhou, leoyu, Cheng Ming Lin Hi Miquel, Miquel Raynal <miquel.raynal@bootlin.com> 於 2024年10月1日 週二 下午6:17寫道： > > Hi Cheng Ming, > > linchengming884@gmail.com wrote on Thu, 5 Sep 2024 13:53:33 +0800: > > > From: Cheng Ming Lin <chengminglin@mxic.com.tw> > > > > When the host ECC fails to correct the data error of NAND device, > > there's a special read for data recovery method which host setups > > for the next read retry mode and may recover the lost data by host > > ECC again. > > > > Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw> > > --- > > drivers/mtd/nand/spi/core.c | 33 +++++++++++++++++++++++++++++++-- > > 1 file changed, 31 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c > > index e0b6715e5dfe..2f21ea926132 100644 > > --- a/drivers/mtd/nand/spi/core.c > > +++ b/drivers/mtd/nand/spi/core.c > > @@ -640,6 +640,7 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from, > > struct nand_io_iter iter; > > bool disable_ecc = false; > > bool ecc_failed = false; > > + u8 retry_mode = 0; > > int ret = 0; > > > > if (ops->mode == MTD_OPS_RAW || !spinand->eccinfo.ooblayout) > > @@ -657,20 +658,45 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from, > > This will no longer apply with continuous support added in. However > please make this only in the non continuous path. I don't think it can > fly in the other. Thanks for your helpful suggestions. > > > if (ret) > > break; > > > > +read_retry: > > ret = spinand_read_page(spinand, &iter.req); > > if (ret < 0 && ret != -EBADMSG) > > break; > > > > - if (ret == -EBADMSG) > > + if (ret == -EBADMSG && spinand->info->fixups) { > > + if (spinand->read_retries && ((retry_mode + 1) < spinand->read_retries)) { > > ++retry_mode? > > + retry_mode++; > > So this can be dropped. > > > + ret = spinand->info->fixups->setup_read_retry(spinand, retry_mode); > > + if (ret < 0) > > + break; > > No, you need to set ecc_failed here. > > > + > > + /* Reset ecc_stats; retry */ > > + mtd->ecc_stats = old_stats; > > + goto read_retry; > > + } else { > > + /* No more retry modes; real failure */ > > + ecc_failed = true; > > + } > > + } else if (ret == -EBADMSG) { > > ecc_failed = true; > > - else > > + } else { > > max_bitflips = max_t(unsigned int, max_bitflips, ret); > > + } > > > > ret = 0; > > ops->retlen += iter.req.datalen; > > ops->oobretlen += iter.req.ooblen; > > + > > + /* Reset to retry mode 0*/ > > + if (retry_mode) { > > retry_mode = 0; > > > + ret = spinand->info->fixups->setup_read_retry(spinand, 0); > > retry_mode); > > > + if (ret < 0) > > + break; > > this if clause is useless. > > > + retry_mode = 0; > > And then this can be dropped from here. > > > + } > > } > > > > + > > Spurious line > > > if (ops->stats) { > > ops->stats->uncorrectable_errors += > > mtd->ecc_stats.failed - old_stats.failed; > > @@ -1095,6 +1121,9 @@ int spinand_match_and_init(struct spinand_device *spinand, > > spinand->flags = table[i].flags; > > spinand->id.len = 1 + table[i].devid.len; > > spinand->select_target = table[i].select_target; > > + spinand->info = info; > > + if (spinand->info->fixups && spinand->info->fixups->init_read_retry) > > + spinand->read_retries = spinand->info->fixups->init_read_retry(spinand); > > Now I get you init. Ok, fine. > > > > > op = spinand_select_op_variant(spinand, > > info->op_variants.read_cache); > Thank you for all your suggestions. I will modify the patch according to your advice. > > Thanks, > Miquèl Thanks, Cheng Ming Lin ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/2] mtd: spi-nand: Add read retry support @ 2024-10-07 5:53 ` Cheng Ming Lin 0 siblings, 0 replies; 23+ messages in thread From: Cheng Ming Lin @ 2024-10-07 5:53 UTC (permalink / raw) To: Miquel Raynal Cc: vigneshr, linux-mtd, linux-kernel, richard, alvinzhou, leoyu, Cheng Ming Lin Hi Miquel, Miquel Raynal <miquel.raynal@bootlin.com> 於 2024年10月1日 週二 下午6:17寫道： > > Hi Cheng Ming, > > linchengming884@gmail.com wrote on Thu, 5 Sep 2024 13:53:33 +0800: > > > From: Cheng Ming Lin <chengminglin@mxic.com.tw> > > > > When the host ECC fails to correct the data error of NAND device, > > there's a special read for data recovery method which host setups > > for the next read retry mode and may recover the lost data by host > > ECC again. > > > > Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw> > > --- > > drivers/mtd/nand/spi/core.c | 33 +++++++++++++++++++++++++++++++-- > > 1 file changed, 31 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c > > index e0b6715e5dfe..2f21ea926132 100644 > > --- a/drivers/mtd/nand/spi/core.c > > +++ b/drivers/mtd/nand/spi/core.c > > @@ -640,6 +640,7 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from, > > struct nand_io_iter iter; > > bool disable_ecc = false; > > bool ecc_failed = false; > > + u8 retry_mode = 0; > > int ret = 0; > > > > if (ops->mode == MTD_OPS_RAW || !spinand->eccinfo.ooblayout) > > @@ -657,20 +658,45 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from, > > This will no longer apply with continuous support added in. However > please make this only in the non continuous path. I don't think it can > fly in the other. Thanks for your helpful suggestions. > > > if (ret) > > break; > > > > +read_retry: > > ret = spinand_read_page(spinand, &iter.req); > > if (ret < 0 && ret != -EBADMSG) > > break; > > > > - if (ret == -EBADMSG) > > + if (ret == -EBADMSG && spinand->info->fixups) { > > + if (spinand->read_retries && ((retry_mode + 1) < spinand->read_retries)) { > > ++retry_mode? > > + retry_mode++; > > So this can be dropped. > > > + ret = spinand->info->fixups->setup_read_retry(spinand, retry_mode); > > + if (ret < 0) > > + break; > > No, you need to set ecc_failed here. > > > + > > + /* Reset ecc_stats; retry */ > > + mtd->ecc_stats = old_stats; > > + goto read_retry; > > + } else { > > + /* No more retry modes; real failure */ > > + ecc_failed = true; > > + } > > + } else if (ret == -EBADMSG) { > > ecc_failed = true; > > - else > > + } else { > > max_bitflips = max_t(unsigned int, max_bitflips, ret); > > + } > > > > ret = 0; > > ops->retlen += iter.req.datalen; > > ops->oobretlen += iter.req.ooblen; > > + > > + /* Reset to retry mode 0*/ > > + if (retry_mode) { > > retry_mode = 0; > > > + ret = spinand->info->fixups->setup_read_retry(spinand, 0); > > retry_mode); > > > + if (ret < 0) > > + break; > > this if clause is useless. > > > + retry_mode = 0; > > And then this can be dropped from here. > > > + } > > } > > > > + > > Spurious line > > > if (ops->stats) { > > ops->stats->uncorrectable_errors += > > mtd->ecc_stats.failed - old_stats.failed; > > @@ -1095,6 +1121,9 @@ int spinand_match_and_init(struct spinand_device *spinand, > > spinand->flags = table[i].flags; > > spinand->id.len = 1 + table[i].devid.len; > > spinand->select_target = table[i].select_target; > > + spinand->info = info; > > + if (spinand->info->fixups && spinand->info->fixups->init_read_retry) > > + spinand->read_retries = spinand->info->fixups->init_read_retry(spinand); > > Now I get you init. Ok, fine. > > > > > op = spinand_select_op_variant(spinand, > > info->op_variants.read_cache); > Thank you for all your suggestions. I will modify the patch according to your advice. > > Thanks, > Miquèl Thanks, Cheng Ming Lin ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/2] mtd: spi-nand: Add read retry support
@ 2024-09-09 12:34 kernel test robot
0 siblings, 0 replies; 23+ messages in thread
From: kernel test robot @ 2024-09-09 12:34 UTC (permalink / raw)
To: oe-kbuild; +Cc: lkp, Dan Carpenter
BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20240905055333.2363358-3-linchengming884@gmail.com>
References: <20240905055333.2363358-3-linchengming884@gmail.com>
TO: Cheng Ming Lin <linchengming884@gmail.com>
TO: miquel.raynal@bootlin.com
TO: vigneshr@ti.com
TO: linux-mtd@lists.infradead.org
TO: linux-kernel@vger.kernel.org
CC: richard@nod.at
CC: alvinzhou@mxic.com.tw
CC: leoyu@mxic.com.tw
CC: Cheng Ming Lin <chengminglin@mxic.com.tw>
Hi Cheng,
kernel test robot noticed the following build warnings:
[auto build test WARNING on mtd/nand/next]
[also build test WARNING on linus/master v6.11-rc7]
[cannot apply to next-20240909]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Cheng-Ming-Lin/mtd-spi-nand-Add-fixups-for-read-retry/20240905-135719
base: https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next
patch link: https://lore.kernel.org/r/20240905055333.2363358-3-linchengming884%40gmail.com
patch subject: [PATCH 2/2] mtd: spi-nand: Add read retry support
:::::: branch date: 4 days ago
:::::: commit date: 4 days ago
config: x86_64-randconfig-161-20240909 (https://download.01.org/0day-ci/archive/20240909/202409092034.1htCE96j-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202409092034.1htCE96j-lkp@intel.com/
smatch warnings:
drivers/mtd/nand/spi/core.c:692 spinand_mtd_read() error: we previously assumed 'spinand->info->fixups' could be null (see line 666)
vim +692 drivers/mtd/nand/spi/core.c
7529df4652482c Peter Pan 2018-06-22 632
7529df4652482c Peter Pan 2018-06-22 633 static int spinand_mtd_read(struct mtd_info *mtd, loff_t from,
7529df4652482c Peter Pan 2018-06-22 634 struct mtd_oob_ops *ops)
7529df4652482c Peter Pan 2018-06-22 635 {
7529df4652482c Peter Pan 2018-06-22 636 struct spinand_device *spinand = mtd_to_spinand(mtd);
7529df4652482c Peter Pan 2018-06-22 637 struct nand_device *nand = mtd_to_nanddev(mtd);
7bea6056927727 Michał Kępień 2022-06-29 638 struct mtd_ecc_stats old_stats;
7529df4652482c Peter Pan 2018-06-22 639 unsigned int max_bitflips = 0;
7529df4652482c Peter Pan 2018-06-22 640 struct nand_io_iter iter;
3d1f08b032dc4e Miquel Raynal 2020-10-01 641 bool disable_ecc = false;
7529df4652482c Peter Pan 2018-06-22 642 bool ecc_failed = false;
ec2cf155ae799c Cheng Ming Lin 2024-09-05 643 u8 retry_mode = 0;
7529df4652482c Peter Pan 2018-06-22 644 int ret = 0;
7529df4652482c Peter Pan 2018-06-22 645
3d1f08b032dc4e Miquel Raynal 2020-10-01 646 if (ops->mode == MTD_OPS_RAW || !spinand->eccinfo.ooblayout)
3d1f08b032dc4e Miquel Raynal 2020-10-01 647 disable_ecc = true;
7529df4652482c Peter Pan 2018-06-22 648
7529df4652482c Peter Pan 2018-06-22 649 mutex_lock(&spinand->lock);
7529df4652482c Peter Pan 2018-06-22 650
7bea6056927727 Michał Kępień 2022-06-29 651 old_stats = mtd->ecc_stats;
7bea6056927727 Michał Kępień 2022-06-29 652
701981cab01696 Miquel Raynal 2020-08-27 653 nanddev_io_for_each_page(nand, NAND_PAGE_READ, from, ops, &iter) {
3d1f08b032dc4e Miquel Raynal 2020-10-01 654 if (disable_ecc)
3d1f08b032dc4e Miquel Raynal 2020-10-01 655 iter.req.mode = MTD_OPS_RAW;
7529df4652482c Peter Pan 2018-06-22 656
3d1f08b032dc4e Miquel Raynal 2020-10-01 657 ret = spinand_select_target(spinand, iter.req.pos.target);
7529df4652482c Peter Pan 2018-06-22 658 if (ret)
7529df4652482c Peter Pan 2018-06-22 659 break;
7529df4652482c Peter Pan 2018-06-22 660
ec2cf155ae799c Cheng Ming Lin 2024-09-05 661 read_retry:
3d1f08b032dc4e Miquel Raynal 2020-10-01 662 ret = spinand_read_page(spinand, &iter.req);
7529df4652482c Peter Pan 2018-06-22 663 if (ret < 0 && ret != -EBADMSG)
7529df4652482c Peter Pan 2018-06-22 664 break;
7529df4652482c Peter Pan 2018-06-22 665
ec2cf155ae799c Cheng Ming Lin 2024-09-05 @666 if (ret == -EBADMSG && spinand->info->fixups) {
ec2cf155ae799c Cheng Ming Lin 2024-09-05 667 if (spinand->read_retries && ((retry_mode + 1) < spinand->read_retries)) {
ec2cf155ae799c Cheng Ming Lin 2024-09-05 668 retry_mode++;
ec2cf155ae799c Cheng Ming Lin 2024-09-05 669 ret = spinand->info->fixups->setup_read_retry(spinand, retry_mode);
ec2cf155ae799c Cheng Ming Lin 2024-09-05 670 if (ret < 0)
ec2cf155ae799c Cheng Ming Lin 2024-09-05 671 break;
ec2cf155ae799c Cheng Ming Lin 2024-09-05 672
ec2cf155ae799c Cheng Ming Lin 2024-09-05 673 /* Reset ecc_stats; retry */
ec2cf155ae799c Cheng Ming Lin 2024-09-05 674 mtd->ecc_stats = old_stats;
ec2cf155ae799c Cheng Ming Lin 2024-09-05 675 goto read_retry;
ec2cf155ae799c Cheng Ming Lin 2024-09-05 676 } else {
ec2cf155ae799c Cheng Ming Lin 2024-09-05 677 /* No more retry modes; real failure */
7529df4652482c Peter Pan 2018-06-22 678 ecc_failed = true;
ec2cf155ae799c Cheng Ming Lin 2024-09-05 679 }
ec2cf155ae799c Cheng Ming Lin 2024-09-05 680 } else if (ret == -EBADMSG) {
ec2cf155ae799c Cheng Ming Lin 2024-09-05 681 ecc_failed = true;
ec2cf155ae799c Cheng Ming Lin 2024-09-05 682 } else {
7529df4652482c Peter Pan 2018-06-22 683 max_bitflips = max_t(unsigned int, max_bitflips, ret);
ec2cf155ae799c Cheng Ming Lin 2024-09-05 684 }
7529df4652482c Peter Pan 2018-06-22 685
b83408b580eccf WeiXiong Liao 2019-06-28 686 ret = 0;
7529df4652482c Peter Pan 2018-06-22 687 ops->retlen += iter.req.datalen;
7529df4652482c Peter Pan 2018-06-22 688 ops->oobretlen += iter.req.ooblen;
ec2cf155ae799c Cheng Ming Lin 2024-09-05 689
ec2cf155ae799c Cheng Ming Lin 2024-09-05 690 /* Reset to retry mode 0*/
ec2cf155ae799c Cheng Ming Lin 2024-09-05 691 if (retry_mode) {
ec2cf155ae799c Cheng Ming Lin 2024-09-05 @692 ret = spinand->info->fixups->setup_read_retry(spinand, 0);
ec2cf155ae799c Cheng Ming Lin 2024-09-05 693 if (ret < 0)
ec2cf155ae799c Cheng Ming Lin 2024-09-05 694 break;
ec2cf155ae799c Cheng Ming Lin 2024-09-05 695 retry_mode = 0;
ec2cf155ae799c Cheng Ming Lin 2024-09-05 696 }
7529df4652482c Peter Pan 2018-06-22 697 }
7529df4652482c Peter Pan 2018-06-22 698
ec2cf155ae799c Cheng Ming Lin 2024-09-05 699
7bea6056927727 Michał Kępień 2022-06-29 700 if (ops->stats) {
7bea6056927727 Michał Kępień 2022-06-29 701 ops->stats->uncorrectable_errors +=
7bea6056927727 Michał Kępień 2022-06-29 702 mtd->ecc_stats.failed - old_stats.failed;
7bea6056927727 Michał Kępień 2022-06-29 703 ops->stats->corrected_bitflips +=
7bea6056927727 Michał Kępień 2022-06-29 704 mtd->ecc_stats.corrected - old_stats.corrected;
7bea6056927727 Michał Kępień 2022-06-29 705 }
7bea6056927727 Michał Kępień 2022-06-29 706
7529df4652482c Peter Pan 2018-06-22 707 mutex_unlock(&spinand->lock);
7529df4652482c Peter Pan 2018-06-22 708
7529df4652482c Peter Pan 2018-06-22 709 if (ecc_failed && !ret)
7529df4652482c Peter Pan 2018-06-22 710 ret = -EBADMSG;
7529df4652482c Peter Pan 2018-06-22 711
7529df4652482c Peter Pan 2018-06-22 712 return ret ? ret : max_bitflips;
7529df4652482c Peter Pan 2018-06-22 713 }
7529df4652482c Peter Pan 2018-06-22 714
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 23+ messages in threadend of thread, other threads:[~2024-10-08 9:23 UTC | newest] Thread overview: 23+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-09-05 5:53 [PATCH 0/2] mtd: spi-nand: Add support for read retry Cheng Ming Lin 2024-09-05 5:53 ` Cheng Ming Lin 2024-09-05 5:53 ` [PATCH 1/2] mtd: spi-nand: Add fixups " Cheng Ming Lin 2024-09-05 5:53 ` Cheng Ming Lin 2024-10-01 9:40 ` Miquel Raynal 2024-10-01 9:40 ` Miquel Raynal 2024-10-07 5:49 ` Cheng Ming Lin 2024-10-07 5:49 ` Cheng Ming Lin 2024-10-07 8:33 ` Miquel Raynal 2024-10-07 8:33 ` Miquel Raynal 2024-10-08 6:25 ` Cheng Ming Lin 2024-10-08 6:25 ` Cheng Ming Lin 2024-10-08 8:55 ` Miquel Raynal 2024-10-08 8:55 ` Miquel Raynal 2024-10-08 9:19 ` Cheng Ming Lin 2024-10-08 9:19 ` Cheng Ming Lin 2024-09-05 5:53 ` [PATCH 2/2] mtd: spi-nand: Add read retry support Cheng Ming Lin 2024-09-05 5:53 ` Cheng Ming Lin 2024-10-01 10:17 ` Miquel Raynal 2024-10-01 10:17 ` Miquel Raynal 2024-10-07 5:53 ` Cheng Ming Lin 2024-10-07 5:53 ` Cheng Ming Lin -- strict thread matches above, loose matches on Subject: below -- 2024-09-09 12:34 kernel test robot
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.