| Message ID | 1557474062-4949-1-git-send-email-masonccyang@mxic.com.tw |
|---|---|
| State | Changes Requested |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | [v1] mtd: rawnand: Add Macronix NAND read retry support | expand |
Hi Mason, Mason Yang <masonccyang@mxic.com.tw> wrote on Fri, 10 May 2019 15:41:02 +0800: > Add a driver for Macronix NAND read retry. "Add support for Macronix NAND read retry."? This is not a "new driver". > > Macronix NAND supports specfical read for data recovery and enabled Macronix NANDs support specific read operation for data recovery, which can be enabled with a SET_FEATURE. > Driver check byte 167 of Vendor Blocks in ONFI parameter page table checks > to see if this high reliability function is support or not. high-reliability function? not sure it is English anyway. supported > > Signed-off-by: Mason Yang <masonccyang@mxic.com.tw> > --- > drivers/mtd/nand/raw/nand_macronix.c | 52 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 52 insertions(+) > > diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c > index 47d8cda..5cd1e5f 100644 > --- a/drivers/mtd/nand/raw/nand_macronix.c > +++ b/drivers/mtd/nand/raw/nand_macronix.c > @@ -17,6 +17,57 @@ > > #include "internals.h" > > +#define MACRONIX_READ_RETRY_BIT BIT(0) > +#define MACRONIX_READ_RETRY_MODE 5 > + > +struct nand_onfi_vendor_macronix { > + u8 reserved[1]; > + u8 reliability_func; > +} __packed; > + > +static int macronix_nand_setup_read_retry(struct nand_chip *chip, int mode) > +{ > + u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = {0}; 0 is not needed here > + int ret; > + > + if (mode > MACRONIX_READ_RETRY_MODE) > + mode = MACRONIX_READ_RETRY_MODE; > + > + feature[0] = mode; > + ret = nand_set_features(chip, ONFI_FEATURE_ADDR_READ_RETRY, feature); Don't you miss to select/deselect the target? > + if (ret) > + pr_err("set feature failed to read retry moded:%d\n", mode); "Failed to set read retry mode: %d\n" I think you can abort the operation with a negative return code in this case. > + > + ret = nand_get_features(chip, ONFI_FEATURE_ADDR_READ_RETRY, feature); If the operation succeeded but the controller cannot get the feature you don't want to abort the operation. You should check if get_features is supported, if yes you can rely on the below test. > + if (ret || feature[0] != mode) > + pr_err("get feature failed to read retry moded:%d(%d)\n", > + mode, feature[0]); "Failed to verify read retry mode..." Also return something negative here. > + > + return ret; And if all went right, return 0 at the end. > +} > + > +static void macronix_nand_onfi_init(struct nand_chip *chip) > +{ > + struct nand_parameters *p = &chip->parameters; > + > + if (p->onfi) { > + struct nand_onfi_vendor_macronix *mxic = > + (void *)p->onfi->vendor; Please put everything on the same line > + > + if (mxic->reliability_func & MACRONIX_READ_RETRY_BIT) { > + chip->read_retries = MACRONIX_READ_RETRY_MODE + 1; Why +1 here, I am missing something? > + chip->setup_read_retry = > + macronix_nand_setup_read_retry; > + if (p->supports_set_get_features) { > + set_bit(ONFI_FEATURE_ADDR_READ_RETRY, > + p->set_feature_list); > + set_bit(ONFI_FEATURE_ADDR_READ_RETRY, > + p->get_feature_list); Please use bitmap_set() > + } > + } > + } > +} > + > /* > * 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 +116,7 @@ static int macronix_nand_init(struct nand_chip *chip) > chip->bbt_options |= NAND_BBT_SCAN2NDPAGE; > > macronix_nand_fix_broken_get_timings(chip); > + macronix_nand_onfi_init(chip); > > return 0; > } Thanks, Miquèl
Hello, Some purely cosmetic suggestions below. On Fri, 10 May 2019 15:41:02 +0800 Mason Yang <masonccyang@mxic.com.tw> wrote: > + if (ret) > + pr_err("set feature failed to read retry moded:%d\n", mode); I don't know what is the policy in the MTD/NAND subsystem, but shouldn't you be using dev_err() instead of pr_err() here to have a nice prefix for the message ? dev_err(&nand_to_mtd(chip)->dev, "set feature ..", mode); > +static void macronix_nand_onfi_init(struct nand_chip *chip) > +{ > + struct nand_parameters *p = &chip->parameters; > + > + if (p->onfi) { Change to: if (!p->onfi) return; This way the rest of the function can save one level of indentation. > + struct nand_onfi_vendor_macronix *mxic = > + (void *)p->onfi->vendor; > + > + if (mxic->reliability_func & MACRONIX_READ_RETRY_BIT) { Change to: if (mxic->reliability_func & MACRONIX_READ_RETRY_BIT == 0) return; And the rest of the function can save one level of indentation. > + chip->read_retries = MACRONIX_READ_RETRY_MODE + 1; > + chip->setup_read_retry = > + macronix_nand_setup_read_retry; > + if (p->supports_set_get_features) { > + set_bit(ONFI_FEATURE_ADDR_READ_RETRY, > + p->set_feature_list); > + set_bit(ONFI_FEATURE_ADDR_READ_RETRY, > + p->get_feature_list); > + } Which will require less wrapping in those lines that are already at the third indentation level. To me, it is also more logical: we exclude the cases we are not interested in and return early, and then if we are still in the case we are interested, we handle it. Best regards, Thomas
Hi Mason, Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote on Fri, 10 May 2019 15:37:04 +0200: > Hello, > > Some purely cosmetic suggestions below. > > On Fri, 10 May 2019 15:41:02 +0800 > Mason Yang <masonccyang@mxic.com.tw> wrote: > > > + if (ret) > > + pr_err("set feature failed to read retry moded:%d\n", mode); > > I don't know what is the policy in the MTD/NAND subsystem, but > shouldn't you be using dev_err() instead of pr_err() here to have a > nice prefix for the message ? > > dev_err(&nand_to_mtd(chip)->dev, "set feature ..", mode); Indeed. You can even dereference an mtd_info object first, then use mtd->dev. > > > +static void macronix_nand_onfi_init(struct nand_chip *chip) > > +{ > > + struct nand_parameters *p = &chip->parameters; > > + > > + if (p->onfi) { > > Change to: > > if (!p->onfi) > return; > > This way the rest of the function can save one level of indentation. > > > + struct nand_onfi_vendor_macronix *mxic = > > + (void *)p->onfi->vendor; > > + > > + if (mxic->reliability_func & MACRONIX_READ_RETRY_BIT) { > > Change to: > > if (mxic->reliability_func & MACRONIX_READ_RETRY_BIT == 0) > return; > > And the rest of the function can save one level of indentation. > > > + chip->read_retries = MACRONIX_READ_RETRY_MODE + 1; > > + chip->setup_read_retry = > > + macronix_nand_setup_read_retry; > > + if (p->supports_set_get_features) { > > + set_bit(ONFI_FEATURE_ADDR_READ_RETRY, > > + p->set_feature_list); > > + set_bit(ONFI_FEATURE_ADDR_READ_RETRY, > > + p->get_feature_list); > > + } > > Which will require less wrapping in those lines that are already at the > third indentation level. > > To me, it is also more logical: we exclude the cases we are not > interested in and return early, and then if we are still in the case we > are interested, we handle it. I definitely agree with these cosmetic changes. Thanks, Miquèl
Hello, Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote on Fri, 10 May 2019 15:37:04 +0200: > Hello, > > Some purely cosmetic suggestions below. > > On Fri, 10 May 2019 15:41:02 +0800 > Mason Yang <masonccyang@mxic.com.tw> wrote: > > > + if (ret) > > + pr_err("set feature failed to read retry moded:%d\n", mode); > > I don't know what is the policy in the MTD/NAND subsystem, but > shouldn't you be using dev_err() instead of pr_err() here to have a > nice prefix for the message ? > > dev_err(&nand_to_mtd(chip)->dev, "set feature ..", mode); Actually, no, manufacturer initializations happens in nand_scan_tail() which runs before mtd_device_register(). At this point, mtd->dev is not yet populated so dev_err() cannot be used. You should keep a pr_err(). Thanks, Miquèl
Hi Miquel, > > > > > > + > > > > > > + if (mxic->reliability_func & MACRONIX_READ_RETRY_BIT) { > > > > > > + chip->read_retries = MACRONIX_READ_RETRY_MODE + 1; > > > > > > > > > > Why +1 here, I am missing something? > > > > > > > > > > > > Without + 1, read retry mode is up to 4 rather than 5. > > > > But this NAND device support read retry mode up to 5. > > > > > > > > > > If there are 5 modes, you need to set 5 to chip->read_retries, not 6. > > > > > > If only 4 modes are used, there is probably a bug in the core that > > > must be fixed, please do not workaround it in the driver! > > > > > > It seems some patches is needed in nand_base.c > > ------------------------------------------------------------------------------------- > > diff --git a/drivers/mtd/nand/raw/nand_base.c > > b/drivers/mtd/nand/raw/nand_base.c > > index ddd396e..56be3a9 100644 > > --- a/drivers/mtd/nand/raw/nand_base.c > > +++ b/drivers/mtd/nand/raw/nand_base.c > > @@ -3101,7 +3101,8 @@ static int nand_setup_read_retry(struct nand_chip > > *chip, int retry_mode) > > { > > pr_debug("setting READ RETRY mode %d\n", retry_mode); > > > > - if (retry_mode >= chip->read_retries) > > + if (retry_mode > chip->read_retries) > > If I understand correctly, chip->read_retries is the total number of > 'modes' so the last valid mode is indeed chip->read_retries -1. > > I thought the problem would come from the core but I was wrong, you > actually need a MACRONIX_NUM_READ_RETRY_MODES set to 6. Please have two > defines if you need both (the first one being something like > MACRONIX_MAXIMUM_READ_RETRY_MODE set to 5). I have checked one of Micron's datasheet and it defined read-retry number = 8, mode 0 ~ 7, 0 mean to disable read-retry. Therefore, I will patch MACRONIX_NUM_READ_RETRY_MODES = 6. 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. =====================================================================
Hi Thomas, > > > > > ------------------------------------------------------------------- > > > > static void macronix_nand_onfi_init(struct nand_chip *chip) > > > > { > > > > struct nand_parameters *p = &chip->parameters; > > > > struct nand_onfi_vendor_macronix *mxic = (void > > > > *)p->onfi->vendor; > > > > > > Why cast to void*, instead of casting directly to struct > > > nand_onfi_vendor_macronix * ? > > > > Due to got a warning: > > > > warning: initialization from incompatible pointer type > > struct nand_onfi_vendor_macronix *mxic = p->onfi->vendor; > > You didn't look at my code, I suggested: > > mxic = (struct nand_onfi_vendor_macronix *) p->info->vendor; Oops, sorry that I did not pay attention to it. Will patch it by your comments. 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); } } 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. =====================================================================
diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c index 47d8cda..5cd1e5f 100644 --- a/drivers/mtd/nand/raw/nand_macronix.c +++ b/drivers/mtd/nand/raw/nand_macronix.c @@ -17,6 +17,57 @@ #include "internals.h" +#define MACRONIX_READ_RETRY_BIT BIT(0) +#define MACRONIX_READ_RETRY_MODE 5 + +struct nand_onfi_vendor_macronix { + u8 reserved[1]; + u8 reliability_func; +} __packed; + +static int macronix_nand_setup_read_retry(struct nand_chip *chip, int mode) +{ + u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = {0}; + int ret; + + if (mode > MACRONIX_READ_RETRY_MODE) + mode = MACRONIX_READ_RETRY_MODE; + + feature[0] = mode; + ret = nand_set_features(chip, ONFI_FEATURE_ADDR_READ_RETRY, feature); + if (ret) + pr_err("set feature failed to read retry moded:%d\n", mode); + + ret = nand_get_features(chip, ONFI_FEATURE_ADDR_READ_RETRY, feature); + if (ret || feature[0] != mode) + pr_err("get feature failed to 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; + + if (p->onfi) { + struct nand_onfi_vendor_macronix *mxic = + (void *)p->onfi->vendor; + + if (mxic->reliability_func & MACRONIX_READ_RETRY_BIT) { + chip->read_retries = MACRONIX_READ_RETRY_MODE + 1; + chip->setup_read_retry = + macronix_nand_setup_read_retry; + if (p->supports_set_get_features) { + set_bit(ONFI_FEATURE_ADDR_READ_RETRY, + p->set_feature_list); + set_bit(ONFI_FEATURE_ADDR_READ_RETRY, + p->get_feature_list); + } + } + } +} + /* * 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 +116,7 @@ static int macronix_nand_init(struct nand_chip *chip) chip->bbt_options |= NAND_BBT_SCAN2NDPAGE; macronix_nand_fix_broken_get_timings(chip); + macronix_nand_onfi_init(chip); return 0; }
Add a driver for Macronix NAND read retry. Macronix NAND supports specfical read for data recovery and enabled it by Set Feature. Driver check byte 167 of Vendor Blocks in ONFI parameter page table to see if this high reliability function is support or not. Signed-off-by: Mason Yang <masonccyang@mxic.com.tw> --- drivers/mtd/nand/raw/nand_macronix.c | 52 ++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+)