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

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(+)

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

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.

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

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

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.

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