* [PATCH v2] mtd: rawnand: Add Macronix NAND read retry support @ 2019-05-17 6:53 Mason Yang 2019-05-20 12:34 ` Miquel Raynal 0 siblings, 1 reply; 5+ messages in thread From: Mason Yang @ 2019-05-17 6:53 UTC (permalink / raw) To: bbrezillon, marek.vasut, linux-kernel, miquel.raynal, richard, dwmw2, computersforpeace, linux-mtd, vigneshr, frieder.schrempf Cc: juliensu, masonccyang, zhengxunli Add support for Macronix NAND read retry. Macronix NANDs support specific read operation for data recovery, which can be enabled/disabled with a SET/GET_FEATURE. Driver checks byte 167 of Vendor Blocks in ONFI parameter page table to see if this high-reliability function is supported. Signed-off-by: Mason Yang <masonccyang@mxic.com.tw> --- drivers/mtd/nand/raw/nand_macronix.c | 57 ++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c index e287e71..1a4dc92 100644 --- a/drivers/mtd/nand/raw/nand_macronix.c +++ b/drivers/mtd/nand/raw/nand_macronix.c @@ -17,6 +17,62 @@ #include "internals.h" +#define MACRONIX_READ_RETRY_BIT BIT(0) +#define MACRONIX_READ_RETRY_MODE 6 + +struct nand_onfi_vendor_macronix { + u8 reserved[1]; + u8 reliability_func; +} __packed; + +/* + * Macronix NANDs support using SET/GET_FEATURES to enter/exit read retry mode + */ +static int macronix_nand_setup_read_retry(struct nand_chip *chip, int mode) +{ + u8 feature[ONFI_SUBFEATURE_PARAM_LEN]; + int ret, feature_addr = ONFI_FEATURE_ADDR_READ_RETRY; + + if (chip->parameters.supports_set_get_features && + test_bit(feature_addr, chip->parameters.set_feature_list) && + test_bit(feature_addr, chip->parameters.get_feature_list)) { + feature[0] = mode; + ret = nand_set_features(chip, feature_addr, feature); + if (ret) + pr_err("Failed to set read retry moded:%d\n", mode); + + ret = nand_get_features(chip, feature_addr, feature); + if (ret || feature[0] != mode) + pr_err("Failed to verify read retry moded:%d(%d)\n", + mode, feature[0]); + } + + return ret; +} + +static void macronix_nand_onfi_init(struct nand_chip *chip) +{ + struct nand_parameters *p = &chip->parameters; + struct nand_onfi_vendor_macronix *mxic; + + if (!p->onfi) + return; + + mxic = (struct nand_onfi_vendor_macronix *)p->onfi->vendor; + if ((mxic->reliability_func & MACRONIX_READ_RETRY_BIT) == 0) + return; + + chip->read_retries = MACRONIX_READ_RETRY_MODE; + chip->setup_read_retry = macronix_nand_setup_read_retry; + + if (p->supports_set_get_features) { + bitmap_set(p->set_feature_list, + ONFI_FEATURE_ADDR_READ_RETRY, 1); + bitmap_set(p->get_feature_list, + ONFI_FEATURE_ADDR_READ_RETRY, 1); + } +} + /* * Macronix AC series does not support using SET/GET_FEATURES to change * the timings unlike what is declared in the parameter page. Unflag @@ -65,6 +121,7 @@ static int macronix_nand_init(struct nand_chip *chip) chip->options |= NAND_BBM_FIRSTPAGE | NAND_BBM_SECONDPAGE; macronix_nand_fix_broken_get_timings(chip); + macronix_nand_onfi_init(chip); return 0; } -- 1.9.1 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] mtd: rawnand: Add Macronix NAND read retry support 2019-05-17 6:53 [PATCH v2] mtd: rawnand: Add Macronix NAND read retry support Mason Yang @ 2019-05-20 12:34 ` Miquel Raynal 2019-05-21 2:42 ` masonccyang 0 siblings, 1 reply; 5+ messages in thread From: Miquel Raynal @ 2019-05-20 12:34 UTC (permalink / raw) To: Mason Yang Cc: vigneshr, bbrezillon, juliensu, richard, linux-kernel, frieder.schrempf, marek.vasut, linux-mtd, computersforpeace, dwmw2, zhengxunli Hi Mason, Mason Yang <masonccyang@mxic.com.tw> wrote on Fri, 17 May 2019 14:53:21 +0800: > Add support for Macronix NAND read retry. > > Macronix NANDs support specific read operation for data recovery, > which can be enabled/disabled with a SET/GET_FEATURE. > Driver checks byte 167 of Vendor Blocks in ONFI parameter page table > to see if this high-reliability function is supported. > > Signed-off-by: Mason Yang <masonccyang@mxic.com.tw> > --- > drivers/mtd/nand/raw/nand_macronix.c | 57 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 57 insertions(+) > > diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c > index e287e71..1a4dc92 100644 > --- a/drivers/mtd/nand/raw/nand_macronix.c > +++ b/drivers/mtd/nand/raw/nand_macronix.c > @@ -17,6 +17,62 @@ > > #include "internals.h" > > +#define MACRONIX_READ_RETRY_BIT BIT(0) > +#define MACRONIX_READ_RETRY_MODE 6 Can you name this define MACRONIX_NUM_READ_RETRY_MODES? > + > +struct nand_onfi_vendor_macronix { > + u8 reserved[1]; Do you need this "[1]" ? > + u8 reliability_func; > +} __packed; > + > +/* > + * Macronix NANDs support using SET/GET_FEATURES to enter/exit read retry mode > + */ > +static int macronix_nand_setup_read_retry(struct nand_chip *chip, int mode) > +{ > + u8 feature[ONFI_SUBFEATURE_PARAM_LEN]; > + int ret, feature_addr = ONFI_FEATURE_ADDR_READ_RETRY; > + > + if (chip->parameters.supports_set_get_features && > + test_bit(feature_addr, chip->parameters.set_feature_list) && > + test_bit(feature_addr, chip->parameters.get_feature_list)) { > + feature[0] = mode; > + ret = nand_set_features(chip, feature_addr, feature); > + if (ret) > + pr_err("Failed to set read retry moded:%d\n", mode); Do you have to call nand_get_features() on error? > + > + ret = nand_get_features(chip, feature_addr, feature); > + if (ret || feature[0] != mode) > + pr_err("Failed to verify read retry moded:%d(%d)\n", > + mode, feature[0]); if ret == 0 but feature[0] != mode, shouldn't you return an error? > + } > + > + return ret; This will produce a Warning at compile time (ret may be used uninitialized). Have you tested it? > +} > + > +static void macronix_nand_onfi_init(struct nand_chip *chip) > +{ > + struct nand_parameters *p = &chip->parameters; > + struct nand_onfi_vendor_macronix *mxic; > + > + if (!p->onfi) > + return; > + > + mxic = (struct nand_onfi_vendor_macronix *)p->onfi->vendor; > + if ((mxic->reliability_func & MACRONIX_READ_RETRY_BIT) == 0) > + return; > + > + chip->read_retries = MACRONIX_READ_RETRY_MODE; > + chip->setup_read_retry = macronix_nand_setup_read_retry; > + > + if (p->supports_set_get_features) { > + bitmap_set(p->set_feature_list, > + ONFI_FEATURE_ADDR_READ_RETRY, 1); > + bitmap_set(p->get_feature_list, > + ONFI_FEATURE_ADDR_READ_RETRY, 1); > + } > +} > + > /* > * Macronix AC series does not support using SET/GET_FEATURES to change > * the timings unlike what is declared in the parameter page. Unflag > @@ -65,6 +121,7 @@ static int macronix_nand_init(struct nand_chip *chip) > chip->options |= NAND_BBM_FIRSTPAGE | NAND_BBM_SECONDPAGE; > > macronix_nand_fix_broken_get_timings(chip); > + macronix_nand_onfi_init(chip); > > return 0; > } Thanks, Miquèl ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] mtd: rawnand: Add Macronix NAND read retry support 2019-05-20 12:34 ` Miquel Raynal @ 2019-05-21 2:42 ` masonccyang 2019-05-21 8:47 ` Miquel Raynal 0 siblings, 1 reply; 5+ messages in thread From: masonccyang @ 2019-05-21 2:42 UTC (permalink / raw) To: Miquel Raynal Cc: vigneshr, bbrezillon, juliensu, richard, linux-kernel, frieder.schrempf, marek.vasut, linux-mtd, computersforpeace, dwmw2, zhengxunli Hi Miquel, > > Add support for Macronix NAND read retry. > > > > Macronix NANDs support specific read operation for data recovery, > > which can be enabled/disabled with a SET/GET_FEATURE. > > Driver checks byte 167 of Vendor Blocks in ONFI parameter page table > > to see if this high-reliability function is supported. > > > > Signed-off-by: Mason Yang <masonccyang@mxic.com.tw> > > --- > > drivers/mtd/nand/raw/nand_macronix.c | 57 ++++++++++++++++++++++++++++++++++++ > > 1 file changed, 57 insertions(+) > > > > diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/ > nand_macronix.c > > index e287e71..1a4dc92 100644 > > --- a/drivers/mtd/nand/raw/nand_macronix.c > > +++ b/drivers/mtd/nand/raw/nand_macronix.c > > @@ -17,6 +17,62 @@ > > > > #include "internals.h" > > > > +#define MACRONIX_READ_RETRY_BIT BIT(0) > > +#define MACRONIX_READ_RETRY_MODE 6 > > Can you name this define MACRONIX_NUM_READ_RETRY_MODES? okay, will fix. > > > + > > +struct nand_onfi_vendor_macronix { > > + u8 reserved[1]; > > Do you need this "[1]" ? okay, just u8 reserved; > > > + u8 reliability_func; > > +} __packed; > > + > > +/* > > + * Macronix NANDs support using SET/GET_FEATURES to enter/exit read retry mode > > + */ > > +static int macronix_nand_setup_read_retry(struct nand_chip *chip, int mode) > > +{ > > + u8 feature[ONFI_SUBFEATURE_PARAM_LEN]; > > + int ret, feature_addr = ONFI_FEATURE_ADDR_READ_RETRY; > > + > > + if (chip->parameters.supports_set_get_features && > > + test_bit(feature_addr, chip->parameters.set_feature_list) && > > + test_bit(feature_addr, chip->parameters.get_feature_list)) { > > + feature[0] = mode; > > + ret = nand_set_features(chip, feature_addr, feature); > > + if (ret) > > + pr_err("Failed to set read retry moded:%d\n", mode); > > Do you have to call nand_get_features() on error? okay > > > + > > + ret = nand_get_features(chip, feature_addr, feature); > > + if (ret || feature[0] != mode) > > + pr_err("Failed to verify read retry moded:%d(%d)\n", > > + mode, feature[0]); > > if ret == 0 but feature[0] != mode, shouldn't you return an error? okay, will fix. > > > + } > > + > > + return ret; > > This will produce a Warning at compile time (ret may be used > uninitialized). Have you tested it? Tool chain I used is "gcc-arm-linux-gnueabi" and no Warning at compile time. Patch it to: -----------------------------------------------------------------------------> static int macronix_nand_setup_read_retry(struct nand_chip *chip, int mode) { u8 feature[ONFI_SUBFEATURE_PARAM_LEN]; int ret, feature_addr = ONFI_FEATURE_ADDR_READ_RETRY; if (chip->parameters.supports_set_get_features && test_bit(feature_addr, chip->parameters.set_feature_list) && test_bit(feature_addr, chip->parameters.get_feature_list)) { feature[0] = mode; ret = nand_set_features(chip, feature_addr, feature); if (ret) { pr_err("Failed to set read retry moded:%d\n", mode); goto err_out; } ret = nand_get_features(chip, feature_addr, feature); if (ret) { pr_err("Failed to get read retry moded:%d\n", mode); goto err_out; } else if (feature[0] != mode) { pr_err("Failed to verify read retry moded:%d(%d)\n", mode, feature[0]); return -EIO; } } err_out: return ret; } -----------------------------------------------------------------------------< thanks & best regards, Mason CONFIDENTIALITY NOTE: This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation. Macronix International Co., Ltd. ===================================================================== ============================================================================ CONFIDENTIALITY NOTE: This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation. Macronix International Co., Ltd. ===================================================================== ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] mtd: rawnand: Add Macronix NAND read retry support 2019-05-21 2:42 ` masonccyang @ 2019-05-21 8:47 ` Miquel Raynal 2019-05-24 7:40 ` masonccyang 0 siblings, 1 reply; 5+ messages in thread From: Miquel Raynal @ 2019-05-21 8:47 UTC (permalink / raw) To: masonccyang Cc: vigneshr, bbrezillon, juliensu, richard, linux-kernel, frieder.schrempf, marek.vasut, linux-mtd, computersforpeace, dwmw2, zhengxunli Hi masonccyang@mxic.com.tw, masonccyang@mxic.com.tw wrote on Tue, 21 May 2019 10:42:06 +0800: > Hi Miquel, > > > > Add support for Macronix NAND read retry. > > > > > > Macronix NANDs support specific read operation for data recovery, > > > which can be enabled/disabled with a SET/GET_FEATURE. > > > Driver checks byte 167 of Vendor Blocks in ONFI parameter page table > > > to see if this high-reliability function is supported. > > > > > > Signed-off-by: Mason Yang <masonccyang@mxic.com.tw> > > > --- > > > drivers/mtd/nand/raw/nand_macronix.c | 57 > ++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 57 insertions(+) > > > > > > diff --git a/drivers/mtd/nand/raw/nand_macronix.c > b/drivers/mtd/nand/raw/ > > nand_macronix.c > > > index e287e71..1a4dc92 100644 > > > --- a/drivers/mtd/nand/raw/nand_macronix.c > > > +++ b/drivers/mtd/nand/raw/nand_macronix.c > > > @@ -17,6 +17,62 @@ > > > > > > #include "internals.h" > > > > > > +#define MACRONIX_READ_RETRY_BIT BIT(0) > > > +#define MACRONIX_READ_RETRY_MODE 6 > > > > Can you name this define MACRONIX_NUM_READ_RETRY_MODES? > > okay, will fix. > > > > > > + > > > +struct nand_onfi_vendor_macronix { > > > + u8 reserved[1]; > > > > Do you need this "[1]" ? > > okay, just u8 reserved; > > > > > > + u8 reliability_func; > > > +} __packed; > > > + > > > +/* > > > + * Macronix NANDs support using SET/GET_FEATURES to enter/exit read > retry mode > > > + */ > > > +static int macronix_nand_setup_read_retry(struct nand_chip *chip, int > mode) > > > +{ > > > + u8 feature[ONFI_SUBFEATURE_PARAM_LEN]; > > > + int ret, feature_addr = ONFI_FEATURE_ADDR_READ_RETRY; > > > + > > > + if (chip->parameters.supports_set_get_features && > > > + test_bit(feature_addr, chip->parameters.set_feature_list) && > > > + test_bit(feature_addr, chip->parameters.get_feature_list)) { > > > + feature[0] = mode; > > > + ret = nand_set_features(chip, feature_addr, feature); > > > + if (ret) > > > + pr_err("Failed to set read retry moded:%d\n", mode); > > > > Do you have to call nand_get_features() on error? > > okay > > > > > > + > > > + ret = nand_get_features(chip, feature_addr, feature); > > > + if (ret || feature[0] != mode) > > > + pr_err("Failed to verify read retry moded:%d(%d)\n", > > > + mode, feature[0]); > > > > if ret == 0 but feature[0] != mode, shouldn't you return an error? > > okay, will fix. > > > > > > + } > > > + > > > + return ret; > > > > This will produce a Warning at compile time (ret may be used > > uninitialized). Have you tested it? > > Tool chain I used is "gcc-arm-linux-gnueabi" and no Warning at compile > time. What's the output of: gcc-arm-linux-gnueabi -v ? > > Patch it to: > -----------------------------------------------------------------------------> > static int macronix_nand_setup_read_retry(struct nand_chip *chip, int > mode) > { > u8 feature[ONFI_SUBFEATURE_PARAM_LEN]; > int ret, feature_addr = ONFI_FEATURE_ADDR_READ_RETRY; > > if (chip->parameters.supports_set_get_features && > test_bit(feature_addr, chip->parameters.set_feature_list) && > test_bit(feature_addr, chip->parameters.get_feature_list)) { > > feature[0] = mode; > ret = nand_set_features(chip, feature_addr, feature); ^ extra space, please be careful with the typos, and run checkpatch.pl --strict before sending patches. > if (ret) { > pr_err("Failed to set read retry moded:%d\n", > mode); > goto err_out; > } > > ret = nand_get_features(chip, feature_addr, feature); > if (ret) { > pr_err("Failed to get read retry moded:%d\n", > mode); > goto err_out; > } else if (feature[0] != mode) { > pr_err("Failed to verify read retry > moded:%d(%d)\n", > mode, feature[0]); > return -EIO; That's not what I meant. You can keep the former condition but if !ret then ret = -EIO for instance. > } > } > > err_out: > return ret; Again, do not jump to a single return call, directly do the return from the point where you want to quit the function. The problem should be that ret may be used uninitialized, the compiler should tell you that. Thanks, Miquèl ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] mtd: rawnand: Add Macronix NAND read retry support 2019-05-21 8:47 ` Miquel Raynal @ 2019-05-24 7:40 ` masonccyang 0 siblings, 0 replies; 5+ messages in thread From: masonccyang @ 2019-05-24 7:40 UTC (permalink / raw) To: Miquel Raynal Cc: vigneshr, bbrezillon, juliensu, richard, linux-kernel, frieder.schrempf, marek.vasut, linux-mtd, computersforpeace, dwmw2, zhengxunli Hi Miquel, > > > > > > > + > > > > + ret = nand_get_features(chip, feature_addr, feature); > > > > + if (ret || feature[0] != mode) > > > > + pr_err("Failed to verify read retry moded:%d(%d)\n", > > > > + mode, feature[0]); > > > > > > if ret == 0 but feature[0] != mode, shouldn't you return an error? > > > > okay, will fix. > > > > > > > > > + } > > > > + > > > > + return ret; > > > > > > This will produce a Warning at compile time (ret may be used > > > uninitialized). Have you tested it? > > > > Tool chain I used is "gcc-arm-linux-gnueabi" and no Warning at compile > > time. > > What's the output of: > gcc-arm-linux-gnueabi -v > ? > Oops, it's gcc 4.8.3 20131111 and kind of obsolete. That's why no Warning at compile time. > > > > Patch it to: > > -----------------------------------------------------------------------------> > > static int macronix_nand_setup_read_retry(struct nand_chip *chip, int > > mode) > > { > > u8 feature[ONFI_SUBFEATURE_PARAM_LEN]; > > int ret, feature_addr = ONFI_FEATURE_ADDR_READ_RETRY; > > > > if (chip->parameters.supports_set_get_features && > > test_bit(feature_addr, chip->parameters.set_feature_list) && > > test_bit(feature_addr, chip->parameters.get_feature_list)) { > > > > feature[0] = mode; > > ret = nand_set_features(chip, feature_addr, feature); > > ^ extra space, please be careful with the > typos, and run checkpatch.pl --strict before > sending patches. > > > if (ret) { > > pr_err("Failed to set read retry moded:%d\n", > > mode); > > goto err_out; > > } > > > > ret = nand_get_features(chip, feature_addr, feature); > > if (ret) { > > pr_err("Failed to get read retry moded:%d\n", > > mode); > > goto err_out; > > } else if (feature[0] != mode) { > > pr_err("Failed to verify read retry > > moded:%d(%d)\n", > > mode, feature[0]); > > return -EIO; > > That's not what I meant. You can keep the former condition but if !ret > then ret = -EIO for instance. > > > } > > } > > > > err_out: > > return ret; > > Again, do not jump to a single return call, directly do the return from > the point where you want to quit the function. > > The problem should be that ret may be used uninitialized, the compiler > should tell you that. got it and thanks for your review. > > Thanks, > Miquèl best regards, Mason CONFIDENTIALITY NOTE: This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation. Macronix International Co., Ltd. ===================================================================== ============================================================================ CONFIDENTIALITY NOTE: This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation. Macronix International Co., Ltd. ===================================================================== ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-05-24 7:41 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-17 6:53 [PATCH v2] mtd: rawnand: Add Macronix NAND read retry support Mason Yang 2019-05-20 12:34 ` Miquel Raynal 2019-05-21 2:42 ` masonccyang 2019-05-21 8:47 ` Miquel Raynal 2019-05-24 7:40 ` masonccyang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).