Re: [PATCH v2] mtd: rawnand: Add Macronix NAND read retry support

[masonccyang]

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

=====================================================================