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

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

[Mason Yang]

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

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;
 }
-- 
1.9.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Mason Yang]

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

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;
 }
-- 
1.9.1

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

[Miquel Raynal]

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Miquel Raynal]

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

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

[Miquel Raynal]

Hi Mason,

masonccyang@mxic.com.tw wrote on Fri, 10 May 2019 16:51:20 +0800:

> Hi Miquel,
> 
> 
> > > 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
> >   
> 
> okay, thanks for your review, will patch it to:
> ------------------------------------------------------------------------
> Add support for Macronix NAND read retry.
> 
> Macronix NANDs support specific read operation for data recovery,
> which can be enabled with a SET_FEATURE.
> Driver checks byte 167 of Vendor Blocks in ONFI parameter page table
> to see if this high-reliability function is supported.
> -------------------------------------------------------------------------

Fine by me.

[...]

> > > +   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?  
> 
> nand_select_target() and nand_deselect_target() are already in 
> nand_do_read_ops().

Right

> 
> >   
> > > +   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.
> >   
> 
> After set feature operation, this NAND device need a get feature command 

You need to add a comment for this.

> or
> SW reset command to exit read retry mode.
> Therefore, set features command followed by get feature command is needed.

In this case you must check first that both set and get are supported.

> 
> > > +
> > > +   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.
> >   
> 
> I thought it has been check in macronix_nand_onfi_init() and set by
> set_bit(ONFI_FEATURE_ADDR_READ_RETRY, p->get_feature_list);

You only checked that the operation can be done, you cannot know in
advance if it will actually succeed.

> 
> right ?
> 
> > > +   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  
> 
> It will over 80 char.

I know, that's fine here.

> 
> >   
> > > +
> > > +      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!


Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Miquel Raynal]

Hi Mason,

masonccyang@mxic.com.tw wrote on Fri, 10 May 2019 16:51:20 +0800:

> Hi Miquel,
> 
> 
> > > 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
> >   
> 
> okay, thanks for your review, will patch it to:
> ------------------------------------------------------------------------
> Add support for Macronix NAND read retry.
> 
> Macronix NANDs support specific read operation for data recovery,
> which can be enabled with a SET_FEATURE.
> Driver checks byte 167 of Vendor Blocks in ONFI parameter page table
> to see if this high-reliability function is supported.
> -------------------------------------------------------------------------

Fine by me.

[...]

> > > +   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?  
> 
> nand_select_target() and nand_deselect_target() are already in 
> nand_do_read_ops().

Right

> 
> >   
> > > +   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.
> >   
> 
> After set feature operation, this NAND device need a get feature command 

You need to add a comment for this.

> or
> SW reset command to exit read retry mode.
> Therefore, set features command followed by get feature command is needed.

In this case you must check first that both set and get are supported.

> 
> > > +
> > > +   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.
> >   
> 
> I thought it has been check in macronix_nand_onfi_init() and set by
> set_bit(ONFI_FEATURE_ADDR_READ_RETRY, p->get_feature_list);

You only checked that the operation can be done, you cannot know in
advance if it will actually succeed.

> 
> right ?
> 
> > > +   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  
> 
> It will over 80 char.

I know, that's fine here.

> 
> >   
> > > +
> > > +      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!


Thanks,
Miquèl

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

[Miquel Raynal]

Hi Mason,

masonccyang@mxic.com.tw wrote on Mon, 13 May 2019 15:47:53 +0800:

> Hi Miquel,
> 
> 
> > >   
> > > >   
> > > > > +   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.
> > > >   
> > > 
> > > After set feature operation, this NAND device need a get feature   
> command 
> > 
> > You need to add a comment for this.  
> 
> okay, will add this comment by next version.
> 
> >   
> > > or
> > > SW reset command to exit read retry mode.
> > > Therefore, set features command followed by get feature command is   
> needed.
> > 
> > In this case you must check first that both set and get are supported.
> >   
> 
> okay, thanks.
> 
> 
> > > > > +
> > > > > +      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).


Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Miquel Raynal]

Hi Mason,

masonccyang@mxic.com.tw wrote on Mon, 13 May 2019 15:47:53 +0800:

> Hi Miquel,
> 
> 
> > >   
> > > >   
> > > > > +   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.
> > > >   
> > > 
> > > After set feature operation, this NAND device need a get feature   
> command 
> > 
> > You need to add a comment for this.  
> 
> okay, will add this comment by next version.
> 
> >   
> > > or
> > > SW reset command to exit read retry mode.
> > > Therefore, set features command followed by get feature command is   
> needed.
> > 
> > In this case you must check first that both set and get are supported.
> >   
> 
> okay, thanks.
> 
> 
> > > > > +
> > > > > +      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).


Thanks,
Miquèl

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

[masonccyang]

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.

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


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[masonccyang]

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.

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

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

[Thomas Petazzoni]

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
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Thomas Petazzoni]

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
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Miquel Raynal]

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Miquel Raynal]

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

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

[Miquel Raynal]

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Miquel Raynal]

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

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

[Thomas Petazzoni]

Hello,

On Mon, 13 May 2019 15:37:39 +0800
masonccyang@mxic.com.tw wrote:

> -------------------------------------------------------------------
>  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 * ?

Also,  you are dereferencing p->info before checking whether it's NULL
or not.

>           if (!p->onfi ||
>               ((mxic->reliability_func & MACRONIX_READ_RETRY_BIT) == 0))
>                   return;

So, the code should be:

	struct nand_onfi_vendor_macronix *mxic;

	if (!p->onfi)
		return;

	mxic = (struct nand_onfi_vendor_macronix *) p->info->vendor;

	if ((mxic->reliability_func & MACRONIX_READ_RETRY_BIT) == 0)
		return;

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Thomas Petazzoni]

Hello,

On Mon, 13 May 2019 15:37:39 +0800
masonccyang@mxic.com.tw wrote:

> -------------------------------------------------------------------
>  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 * ?

Also,  you are dereferencing p->info before checking whether it's NULL
or not.

>           if (!p->onfi ||
>               ((mxic->reliability_func & MACRONIX_READ_RETRY_BIT) == 0))
>                   return;

So, the code should be:

	struct nand_onfi_vendor_macronix *mxic;

	if (!p->onfi)
		return;

	mxic = (struct nand_onfi_vendor_macronix *) p->info->vendor;

	if ((mxic->reliability_func & MACRONIX_READ_RETRY_BIT) == 0)
		return;

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Thomas Petazzoni]

Hello,

On Tue, 14 May 2019 09:53:16 +0800
masonccyang@mxic.com.tw wrote:

> > > -------------------------------------------------------------------
> > >  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;

I.e, you indeed still need a cast, because p->info->vendor is a u8[].
But instead of casting to void*, and then implicitly casting to struct
nand_onfi_vendor_macronix *, I suggest to cast directly to struct
nand_onfi_vendor_macronix *.

> > >           if (!p->onfi ||
> > >               ((mxic->reliability_func & MACRONIX_READ_RETRY_BIT) ==   
> 0))
> > >                   return;  
> > 
> > So, the code should be:
> > 
> >    struct nand_onfi_vendor_macronix *mxic;
> > 
> >    if (!p->onfi)
> >       return;
> > 
> >    mxic = (struct nand_onfi_vendor_macronix *) p->info->vendor;
> > 
> >    if ((mxic->reliability_func & MACRONIX_READ_RETRY_BIT) == 0)
> >       return;  
> 
> Also got a warning:
> 
> warning: ISO C90 forbids mixed declarations and code 
> [-Wdeclaration-after-statement]

No, you don't get this warning if you use my code. You get this warning
if you declare and initialized the "mxic" variable at the same location.

>  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;

You are dereferencing p->info...

> 
>          if (!p->onfi)
>                  return;

... before you check it is NULL. This is wrong.

Please check again the code I sent in my previous e-mail:

    struct nand_onfi_vendor_macronix *mxic;
 
    if (!p->onfi)
       return;
 
    mxic = (struct nand_onfi_vendor_macronix *) p->info->vendor;
 
    if ((mxic->reliability_func & MACRONIX_READ_RETRY_BIT) == 0)
       return;  

Best regards,

Thomas Petazzoni
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Thomas Petazzoni]

Hello,

On Tue, 14 May 2019 09:53:16 +0800
masonccyang@mxic.com.tw wrote:

> > > -------------------------------------------------------------------
> > >  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;

I.e, you indeed still need a cast, because p->info->vendor is a u8[].
But instead of casting to void*, and then implicitly casting to struct
nand_onfi_vendor_macronix *, I suggest to cast directly to struct
nand_onfi_vendor_macronix *.

> > >           if (!p->onfi ||
> > >               ((mxic->reliability_func & MACRONIX_READ_RETRY_BIT) ==   
> 0))
> > >                   return;  
> > 
> > So, the code should be:
> > 
> >    struct nand_onfi_vendor_macronix *mxic;
> > 
> >    if (!p->onfi)
> >       return;
> > 
> >    mxic = (struct nand_onfi_vendor_macronix *) p->info->vendor;
> > 
> >    if ((mxic->reliability_func & MACRONIX_READ_RETRY_BIT) == 0)
> >       return;  
> 
> Also got a warning:
> 
> warning: ISO C90 forbids mixed declarations and code 
> [-Wdeclaration-after-statement]

No, you don't get this warning if you use my code. You get this warning
if you declare and initialized the "mxic" variable at the same location.

>  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;

You are dereferencing p->info...

> 
>          if (!p->onfi)
>                  return;

... before you check it is NULL. This is wrong.

Please check again the code I sent in my previous e-mail:

    struct nand_onfi_vendor_macronix *mxic;
 
    if (!p->onfi)
       return;
 
    mxic = (struct nand_onfi_vendor_macronix *) p->info->vendor;
 
    if ((mxic->reliability_func & MACRONIX_READ_RETRY_BIT) == 0)
       return;  

Best regards,

Thomas Petazzoni
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[masonccyang]

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.

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


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[masonccyang]

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.

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