[PATCH RFC 1/3] mtd: rawnand: Add support manufacturer postponed initialization

[PATCH RFC 1/3] mtd: rawnand: Add support manufacturer postponed initialization

[Mason Yang]

Manufacturer postponed initialization is for MTD default call-back
function replacement for vendor soecific operation, i.e.,
_lock/_unlock, _suspend/_resume and so on.

Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
---
 drivers/mtd/nand/raw/internals.h |  4 ++++
 drivers/mtd/nand/raw/nand_base.c | 19 +++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h
index cba6fe7..505dd46 100644
--- a/drivers/mtd/nand/raw/internals.h
+++ b/drivers/mtd/nand/raw/internals.h
@@ -42,6 +42,9 @@
  *	     is here to let vendor specific code release those resources.
  * @fixup_onfi_param_page: apply vendor specific fixups to the ONFI parameter
  *			   page. This is called after the checksum is verified.
+ * @post_init: postponed initialization is for MTD default call-back function
+ *	       replacement for vendor specific operation i.e., _lock/_unlock,
+ *	       _suspend/_resume and so on.
  */
 struct nand_manufacturer_ops {
 	void (*detect)(struct nand_chip *chip);
@@ -49,6 +52,7 @@ struct nand_manufacturer_ops {
 	void (*cleanup)(struct nand_chip *chip);
 	void (*fixup_onfi_param_page)(struct nand_chip *chip,
 				      struct nand_onfi_params *p);
+	void (*post_init)(struct nand_chip *chip);
 };
 
 /**
diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 91f046d..7835b81 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -4619,6 +4619,21 @@ static int nand_manufacturer_init(struct nand_chip *chip)
 }
 
 /*
+ * Manufacturer postponed initialization. This function is called for all NANDs
+ * whose MTD default call-back function replacement is needed.
+ * Manufacturer drivers should put all their specific postponed initialization
+ * code in their ->post_init() hook.
+ */
+static void nand_manufacturer_post_init(struct nand_chip *chip)
+{
+	if (!chip->manufacturer.desc || !chip->manufacturer.desc->ops ||
+	    !chip->manufacturer.desc->ops->post_init)
+		return;
+
+	return chip->manufacturer.desc->ops->post_init(chip);
+}
+
+/*
  * Manufacturer cleanup. This function is called for all NANDs including
  * ONFI and JEDEC compliant ones.
  * Manufacturer drivers should put all their specific cleanup code in their
@@ -5812,6 +5827,10 @@ static int nand_scan_tail(struct nand_chip *chip)
 			goto err_nanddev_cleanup;
 	}
 
+	nand_select_target(chip, 0);
+	nand_manufacturer_post_init(chip);
+	nand_deselect_target(chip);
+
 	/* Check, if we should skip the bad block table scan */
 	if (chip->options & NAND_SKIP_BBTSCAN)
 		return 0;
-- 
1.9.1


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

[PATCH RFC 2/3] mtd: rawnand: Add support Macronix Block Protection function

[Mason Yang]

Macronix AC series support using SET/GET_FEATURES to change
Block Protection and Unprotection.

MTD default _lock/_unlock function replacement by manufacturer
postponed initialization.

Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
---
 drivers/mtd/nand/raw/nand_macronix.c | 80 +++++++++++++++++++++++++++++++++---
 1 file changed, 75 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c
index 58511ae..991c636 100644
--- a/drivers/mtd/nand/raw/nand_macronix.c
+++ b/drivers/mtd/nand/raw/nand_macronix.c
@@ -11,6 +11,10 @@
 #define MACRONIX_READ_RETRY_BIT BIT(0)
 #define MACRONIX_NUM_READ_RETRY_MODES 6
 
+#define ONFI_FEATURE_ADDR_MXIC_PROTECTION 0xA0
+#define MXIC_BLOCK_PROTECTION_ALL_LOCK 0x38
+#define MXIC_BLOCK_PROTECTION_ALL_UNLOCK 0x0
+
 struct nand_onfi_vendor_macronix {
 	u8 reserved;
 	u8 reliability_func;
@@ -57,10 +61,7 @@ static void macronix_nand_onfi_init(struct nand_chip *chip)
  * the timings unlike what is declared in the parameter page. Unflag
  * this feature to avoid unnecessary downturns.
  */
-static void macronix_nand_fix_broken_get_timings(struct nand_chip *chip)
-{
-	unsigned int i;
-	static const char * const broken_get_timings[] = {
+static const char * const broken_get_timings[] = {
 		"MX30LF1G18AC",
 		"MX30LF1G28AC",
 		"MX30LF2G18AC",
@@ -75,7 +76,11 @@ static void macronix_nand_fix_broken_get_timings(struct nand_chip *chip)
 		"MX30UF4G18AC",
 		"MX30UF4G16AC",
 		"MX30UF4G28AC",
-	};
+};
+
+static void macronix_nand_fix_broken_get_timings(struct nand_chip *chip)
+{
+	unsigned int i;
 
 	if (!chip->parameters.supports_set_get_features)
 		return;
@@ -105,6 +110,71 @@ static int macronix_nand_init(struct nand_chip *chip)
 	return 0;
 }
 
+static int mxic_nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	u8 feature[ONFI_SUBFEATURE_PARAM_LEN];
+	int ret;
+
+	feature[0] = MXIC_BLOCK_PROTECTION_ALL_LOCK;
+	nand_select_target(chip, 0);
+	ret = nand_set_features(chip, ONFI_FEATURE_ADDR_MXIC_PROTECTION,
+				feature);
+	nand_deselect_target(chip);
+	if (ret)
+		pr_err("%s all blocks failed\n", __func__);
+
+	return ret;
+}
+
+static int mxic_nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	u8 feature[ONFI_SUBFEATURE_PARAM_LEN];
+	int ret;
+
+	feature[0] = MXIC_BLOCK_PROTECTION_ALL_UNLOCK;
+	nand_select_target(chip, 0);
+	ret = nand_set_features(chip, ONFI_FEATURE_ADDR_MXIC_PROTECTION,
+				feature);
+	nand_deselect_target(chip);
+	if (ret)
+		pr_err("%s all blocks failed\n", __func__);
+
+	return ret;
+}
+
+/*
+ * Macronix AC series support using SET/GET_FEATURES to change
+ * Block Protection and Unprotection.
+ *
+ * MTD call-back function replacement by manufacturer postponed
+ * initialization.
+ */
+static void macronix_nand_post_init(struct nand_chip *chip)
+{
+	unsigned int i, blockprotected = 0;
+	struct mtd_info *mtd = nand_to_mtd(chip);
+
+	for (i = 0; i < ARRAY_SIZE(broken_get_timings); i++) {
+		if (!strcmp(broken_get_timings[i], chip->parameters.model)) {
+			blockprotected = 1;
+			break;
+		}
+	}
+
+	if (blockprotected && chip->parameters.supports_set_get_features) {
+		bitmap_set(chip->parameters.set_feature_list,
+			   ONFI_FEATURE_ADDR_MXIC_PROTECTION, 1);
+		bitmap_set(chip->parameters.get_feature_list,
+			   ONFI_FEATURE_ADDR_MXIC_PROTECTION, 1);
+
+		mtd->_lock = mxic_nand_lock;
+		mtd->_unlock = mxic_nand_unlock;
+	}
+}
+
 const struct nand_manufacturer_ops macronix_nand_manuf_ops = {
 	.init = macronix_nand_init,
+	.post_init = macronix_nand_post_init,
 };
-- 
1.9.1


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

Re: [PATCH RFC 2/3] mtd: rawnand: Add support Macronix Block Protection function

[Miquel Raynal]

Hi Mason,

Mason Yang <masonccyang@mxic.com.tw> wrote on Wed, 18 Sep 2019 15:56:25
+0800:

> Macronix AC series support using SET/GET_FEATURES to change
> Block Protection and Unprotection.
> 
> MTD default _lock/_unlock function replacement by manufacturer
> postponed initialization.

Why would we do that?

Anyway your solution looks overkill, if we ever decide to
implement these hooks for raw nand, it is better just to not
overwrite them in nand_scan_tail() if they have been filled
previously (ie. by the manufacturer code).

> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> ---
>  drivers/mtd/nand/raw/nand_macronix.c | 80 +++++++++++++++++++++++++++++++++---
>  1 file changed, 75 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c
> index 58511ae..991c636 100644
> --- a/drivers/mtd/nand/raw/nand_macronix.c
> +++ b/drivers/mtd/nand/raw/nand_macronix.c
> @@ -11,6 +11,10 @@
>  #define MACRONIX_READ_RETRY_BIT BIT(0)
>  #define MACRONIX_NUM_READ_RETRY_MODES 6
>  
> +#define ONFI_FEATURE_ADDR_MXIC_PROTECTION 0xA0
> +#define MXIC_BLOCK_PROTECTION_ALL_LOCK 0x38
> +#define MXIC_BLOCK_PROTECTION_ALL_UNLOCK 0x0
> +
>  struct nand_onfi_vendor_macronix {
>  	u8 reserved;
>  	u8 reliability_func;
> @@ -57,10 +61,7 @@ static void macronix_nand_onfi_init(struct nand_chip *chip)
>   * the timings unlike what is declared in the parameter page. Unflag
>   * this feature to avoid unnecessary downturns.
>   */
> -static void macronix_nand_fix_broken_get_timings(struct nand_chip *chip)
> -{
> -	unsigned int i;
> -	static const char * const broken_get_timings[] = {
> +static const char * const broken_get_timings[] = {
>  		"MX30LF1G18AC",
>  		"MX30LF1G28AC",
>  		"MX30LF2G18AC",
> @@ -75,7 +76,11 @@ static void macronix_nand_fix_broken_get_timings(struct nand_chip *chip)
>  		"MX30UF4G18AC",
>  		"MX30UF4G16AC",
>  		"MX30UF4G28AC",
> -	};
> +};
> +
> +static void macronix_nand_fix_broken_get_timings(struct nand_chip *chip)
> +{
> +	unsigned int i;
>  
>  	if (!chip->parameters.supports_set_get_features)
>  		return;
> @@ -105,6 +110,71 @@ static int macronix_nand_init(struct nand_chip *chip)
>  	return 0;
>  }
>  
> +static int mxic_nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	u8 feature[ONFI_SUBFEATURE_PARAM_LEN];
> +	int ret;
> +
> +	feature[0] = MXIC_BLOCK_PROTECTION_ALL_LOCK;
> +	nand_select_target(chip, 0);
> +	ret = nand_set_features(chip, ONFI_FEATURE_ADDR_MXIC_PROTECTION,
> +				feature);
> +	nand_deselect_target(chip);
> +	if (ret)
> +		pr_err("%s all blocks failed\n", __func__);
> +
> +	return ret;
> +}
> +
> +static int mxic_nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	u8 feature[ONFI_SUBFEATURE_PARAM_LEN];
> +	int ret;
> +
> +	feature[0] = MXIC_BLOCK_PROTECTION_ALL_UNLOCK;
> +	nand_select_target(chip, 0);
> +	ret = nand_set_features(chip, ONFI_FEATURE_ADDR_MXIC_PROTECTION,
> +				feature);
> +	nand_deselect_target(chip);
> +	if (ret)
> +		pr_err("%s all blocks failed\n", __func__);
> +
> +	return ret;
> +}
> +
> +/*
> + * Macronix AC series support using SET/GET_FEATURES to change
> + * Block Protection and Unprotection.
> + *
> + * MTD call-back function replacement by manufacturer postponed
> + * initialization.
> + */
> +static void macronix_nand_post_init(struct nand_chip *chip)
> +{
> +	unsigned int i, blockprotected = 0;
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +
> +	for (i = 0; i < ARRAY_SIZE(broken_get_timings); i++) {
> +		if (!strcmp(broken_get_timings[i], chip->parameters.model)) {
> +			blockprotected = 1;
> +			break;
> +		}
> +	}
> +
> +	if (blockprotected && chip->parameters.supports_set_get_features) {
> +		bitmap_set(chip->parameters.set_feature_list,
> +			   ONFI_FEATURE_ADDR_MXIC_PROTECTION, 1);
> +		bitmap_set(chip->parameters.get_feature_list,
> +			   ONFI_FEATURE_ADDR_MXIC_PROTECTION, 1);
> +
> +		mtd->_lock = mxic_nand_lock;
> +		mtd->_unlock = mxic_nand_unlock;
> +	}
> +}
> +
>  const struct nand_manufacturer_ops macronix_nand_manuf_ops = {
>  	.init = macronix_nand_init,
> +	.post_init = macronix_nand_post_init,
>  };


Thanks,
Miquèl

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

Re: [PATCH RFC 2/3] mtd: rawnand: Add support Macronix Block Protection function

[Miquel Raynal]

Hi Mason,

Miquel Raynal <miquel.raynal@bootlin.com> wrote on Mon, 7 Oct 2019
10:45:11 +0200:

> Hi Mason,
> 
> Mason Yang <masonccyang@mxic.com.tw> wrote on Wed, 18 Sep 2019 15:56:25
> +0800:
> 
> > Macronix AC series support using SET/GET_FEATURES to change
> > Block Protection and Unprotection.
> > 
> > MTD default _lock/_unlock function replacement by manufacturer
> > postponed initialization.  
> 
> Why would we do that?
> 
> Anyway your solution looks overkill, if we ever decide to
> implement these hooks for raw nand, it is better just to not
> overwrite them in nand_scan_tail() if they have been filled
> previously (ie. by the manufacturer code).

Actually you should add two NAND hooks that do the interface with the
MTD hooks. In the NAND hooks, check that the request is to lock all the
device, otherwise return -ENOTSUPP.

Then fill-in these two hooks from the manufacturer code, without the
postponed init.

> 
> > Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> > ---
> >  drivers/mtd/nand/raw/nand_macronix.c | 80 +++++++++++++++++++++++++++++++++---
> >  1 file changed, 75 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c
> > index 58511ae..991c636 100644
> > --- a/drivers/mtd/nand/raw/nand_macronix.c
> > +++ b/drivers/mtd/nand/raw/nand_macronix.c
> > @@ -11,6 +11,10 @@
> >  #define MACRONIX_READ_RETRY_BIT BIT(0)
> >  #define MACRONIX_NUM_READ_RETRY_MODES 6
> >  
> > +#define ONFI_FEATURE_ADDR_MXIC_PROTECTION 0xA0
> > +#define MXIC_BLOCK_PROTECTION_ALL_LOCK 0x38
> > +#define MXIC_BLOCK_PROTECTION_ALL_UNLOCK 0x0
> > +
> >  struct nand_onfi_vendor_macronix {
> >  	u8 reserved;
> >  	u8 reliability_func;
> > @@ -57,10 +61,7 @@ static void macronix_nand_onfi_init(struct nand_chip *chip)
> >   * the timings unlike what is declared in the parameter page. Unflag
> >   * this feature to avoid unnecessary downturns.
> >   */
> > -static void macronix_nand_fix_broken_get_timings(struct nand_chip *chip)
> > -{
> > -	unsigned int i;
> > -	static const char * const broken_get_timings[] = {
> > +static const char * const broken_get_timings[] = {
> >  		"MX30LF1G18AC",
> >  		"MX30LF1G28AC",
> >  		"MX30LF2G18AC",
> > @@ -75,7 +76,11 @@ static void macronix_nand_fix_broken_get_timings(struct nand_chip *chip)
> >  		"MX30UF4G18AC",
> >  		"MX30UF4G16AC",
> >  		"MX30UF4G28AC",
> > -	};
> > +};
> > +
> > +static void macronix_nand_fix_broken_get_timings(struct nand_chip *chip)
> > +{
> > +	unsigned int i;
> >  
> >  	if (!chip->parameters.supports_set_get_features)
> >  		return;
> > @@ -105,6 +110,71 @@ static int macronix_nand_init(struct nand_chip *chip)
> >  	return 0;
> >  }
> >  
> > +static int mxic_nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> > +{
> > +	struct nand_chip *chip = mtd_to_nand(mtd);
> > +	u8 feature[ONFI_SUBFEATURE_PARAM_LEN];
> > +	int ret;
> > +
> > +	feature[0] = MXIC_BLOCK_PROTECTION_ALL_LOCK;
> > +	nand_select_target(chip, 0);
> > +	ret = nand_set_features(chip, ONFI_FEATURE_ADDR_MXIC_PROTECTION,
> > +				feature);
> > +	nand_deselect_target(chip);
> > +	if (ret)
> > +		pr_err("%s all blocks failed\n", __func__);
> > +
> > +	return ret;
> > +}
> > +
> > +static int mxic_nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> > +{
> > +	struct nand_chip *chip = mtd_to_nand(mtd);
> > +	u8 feature[ONFI_SUBFEATURE_PARAM_LEN];
> > +	int ret;
> > +
> > +	feature[0] = MXIC_BLOCK_PROTECTION_ALL_UNLOCK;
> > +	nand_select_target(chip, 0);
> > +	ret = nand_set_features(chip, ONFI_FEATURE_ADDR_MXIC_PROTECTION,
> > +				feature);
> > +	nand_deselect_target(chip);
> > +	if (ret)
> > +		pr_err("%s all blocks failed\n", __func__);
> > +
> > +	return ret;
> > +}
> > +
> > +/*
> > + * Macronix AC series support using SET/GET_FEATURES to change
> > + * Block Protection and Unprotection.
> > + *
> > + * MTD call-back function replacement by manufacturer postponed
> > + * initialization.
> > + */
> > +static void macronix_nand_post_init(struct nand_chip *chip)
> > +{
> > +	unsigned int i, blockprotected = 0;
> > +	struct mtd_info *mtd = nand_to_mtd(chip);
> > +
> > +	for (i = 0; i < ARRAY_SIZE(broken_get_timings); i++) {
> > +		if (!strcmp(broken_get_timings[i], chip->parameters.model)) {
> > +			blockprotected = 1;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (blockprotected && chip->parameters.supports_set_get_features) {
> > +		bitmap_set(chip->parameters.set_feature_list,
> > +			   ONFI_FEATURE_ADDR_MXIC_PROTECTION, 1);
> > +		bitmap_set(chip->parameters.get_feature_list,
> > +			   ONFI_FEATURE_ADDR_MXIC_PROTECTION, 1);
> > +
> > +		mtd->_lock = mxic_nand_lock;
> > +		mtd->_unlock = mxic_nand_unlock;
> > +	}
> > +}
> > +
> >  const struct nand_manufacturer_ops macronix_nand_manuf_ops = {
> >  	.init = macronix_nand_init,
> > +	.post_init = macronix_nand_post_init,
> >  };  
> 
> 
> Thanks,
> Miquèl

Thanks,
Miquèl

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

Re: [PATCH RFC 2/3] mtd: rawnand: Add support Macronix Block Protection function

[masonccyang]

Hi Miquel,

> > 
> > > Macronix AC series support using SET/GET_FEATURES to change
> > > Block Protection and Unprotection.
> > > 
> > > MTD default _lock/_unlock function replacement by manufacturer
> > > postponed initialization. 
> > 
> > Why would we do that?
> > 
> > Anyway your solution looks overkill, if we ever decide to
> > implement these hooks for raw nand, it is better just to not
> > overwrite them in nand_scan_tail() if they have been filled
> > previously (ie. by the manufacturer code).
> 
> Actually you should add two NAND hooks that do the interface with the
> MTD hooks. In the NAND hooks, check that the request is to lock all the
> device, otherwise return -ENOTSUPP.

sorry, can't get your point.

Because the NAND entire chip will be protected if PT(protection) pin 
is active high at power-on.

> 
> Then fill-in these two hooks from the manufacturer code, without the
> postponed init.
> 

But in the final of nand_scan_tail(), mtd->_lock/_unlock will be
filled by NULL, right ?

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 RFC 2/3] mtd: rawnand: Add support Macronix Block Protection function

[Miquel Raynal]

Hi Mason,

masonccyang@mxic.com.tw wrote on Tue, 8 Oct 2019 10:33:11 +0800:

> Hi Miquel,
> 
> > >   
> > > > Macronix AC series support using SET/GET_FEATURES to change
> > > > Block Protection and Unprotection.
> > > > 
> > > > MTD default _lock/_unlock function replacement by manufacturer
> > > > postponed initialization.   
> > > 
> > > Why would we do that?
> > > 
> > > Anyway your solution looks overkill, if we ever decide to
> > > implement these hooks for raw nand, it is better just to not
> > > overwrite them in nand_scan_tail() if they have been filled
> > > previously (ie. by the manufacturer code).  
> > 
> > Actually you should add two NAND hooks that do the interface with the
> > MTD hooks. In the NAND hooks, check that the request is to lock all the
> > device, otherwise return -ENOTSUPP.  
> 
> sorry, can't get your point.
> 
> Because the NAND entire chip will be protected if PT(protection) pin 
> is active high at power-on.

In your implementation of the locking, you should check that the
locking request is over the entire device, unless you can lock a
smaller portion of course.

> 
> > 
> > Then fill-in these two hooks from the manufacturer code, without the
> > postponed init.
> >   
> 
> But in the final of nand_scan_tail(), mtd->_lock/_unlock will be
> filled by NULL, right ?

The NAND core should set mtd->_lock/_unlock() to NAND specific hooks so
that the MTD layer is abstracted and and drivers do not see it. Then,
in the NAND helper, either there is no specific hook defined by a
manufacturer driver and you return -ENOTSUPP, or you execute the
defined hook.

Thanks,
Miquèl

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

Re: [PATCH RFC 2/3] mtd: rawnand: Add support Macronix Block Protection function

[masonccyang]

Hi Miquel,

> > > > 
> > > > > Macronix AC series support using SET/GET_FEATURES to change
> > > > > Block Protection and Unprotection.
> > > > > 
> > > > > MTD default _lock/_unlock function replacement by manufacturer
> > > > > postponed initialization. 
> > > > 
> > > > Why would we do that?
> > > > 
> > > > Anyway your solution looks overkill, if we ever decide to
> > > > implement these hooks for raw nand, it is better just to not
> > > > overwrite them in nand_scan_tail() if they have been filled
> > > > previously (ie. by the manufacturer code). 
> > > 
> > > Actually you should add two NAND hooks that do the interface with 
the
> > > MTD hooks. In the NAND hooks, check that the request is to lock all 
the
> > > device, otherwise return -ENOTSUPP. 
> > 
> > sorry, can't get your point.
> > 
> > Because the NAND entire chip will be protected if PT(protection) pin 
> > is active high at power-on.
> 
> In your implementation of the locking, you should check that the
> locking request is over the entire device, unless you can lock a
> smaller portion of course.

yes, I can lock a smaller portion.
And at the power-on of device with PT pin at high voltage, all blocks are 
locked.
They have to be unlocked by set feature command.

> 
> > 
> > > 
> > > Then fill-in these two hooks from the manufacturer code, without the
> > > postponed init.
> > > 
> > 
> > But in the final of nand_scan_tail(), mtd->_lock/_unlock will be
> > filled by NULL, right ?
> 
> The NAND core should set mtd->_lock/_unlock() to NAND specific hooks so
> that the MTD layer is abstracted and and drivers do not see it. Then,
> in the NAND helper, either there is no specific hook defined by a
> manufacturer driver and you return -ENOTSUPP, or you execute the
> defined hook.

okay, patch specific manufacturer _lock/_unlock driver
in nand_manufacturer_init();

and in the final of nand_scan_tail()
if (!mtd->_lock)
        mtd->_lock = NULL;
if (!mtd->_unlock)
        mtd->_unlock = NULL;

thanks for your time and comments.
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 RFC 2/3] mtd: rawnand: Add support Macronix Block Protection function

[masonccyang]

Hi Miquel,


> > > > Then fill-in these two hooks from the manufacturer code, without 
the
> > > > postponed init.
> > > > 
> > > 
> > > But in the final of nand_scan_tail(), mtd->_lock/_unlock will be
> > > filled by NULL, right ?
> > 
> > The NAND core should set mtd->_lock/_unlock() to NAND specific hooks 
so
> > that the MTD layer is abstracted and and drivers do not see it. Then,
> > in the NAND helper, either there is no specific hook defined by a
> > manufacturer driver and you return -ENOTSUPP, or you execute the
> > defined hook.
> 
> okay, patch specific manufacturer _lock/_unlock driver
> in nand_manufacturer_init();
> 
> and in the final of nand_scan_tail()
> if (!mtd->_lock)
>  mtd->_lock = NULL;
> if (!mtd->_unlock)
>  mtd->_unlock = NULL;
 

I'm still considering of post_init() in nand_scan_tail() for
MTD layer default call-back function replacement because
there would be more call-back functions need it.
i.e., 
MTD->_lock/_unlokc
MTD->_suspend/_resume
NTD->_point/_unpoint
...


actually, my patch series are including MTD->_locl/_unlock and 
MTD->_suspend/_resume. how do you think ?


thanks for your time & comments.
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 RFC 2/3] mtd: rawnand: Add support Macronix Block Protection function

[Boris Brezillon]

On Mon, 21 Oct 2019 15:23:57 +0800
masonccyang@mxic.com.tw wrote:

> Hi Miquel,
> 
> 
> > > > > Then fill-in these two hooks from the manufacturer code, without   
> the
> > > > > postponed init.
> > > > >   
> > > > 
> > > > But in the final of nand_scan_tail(), mtd->_lock/_unlock will be
> > > > filled by NULL, right ?  
> > > 
> > > The NAND core should set mtd->_lock/_unlock() to NAND specific hooks   
> so
> > > that the MTD layer is abstracted and and drivers do not see it. Then,
> > > in the NAND helper, either there is no specific hook defined by a
> > > manufacturer driver and you return -ENOTSUPP, or you execute the
> > > defined hook.  
> > 
> > okay, patch specific manufacturer _lock/_unlock driver
> > in nand_manufacturer_init();
> > 
> > and in the final of nand_scan_tail()
> > if (!mtd->_lock)
> >  mtd->_lock = NULL;
> > if (!mtd->_unlock)
> >  mtd->_unlock = NULL;  
>  
> 
> I'm still considering of post_init() in nand_scan_tail() for
> MTD layer default call-back function replacement because
> there would be more call-back functions need it.
> i.e., 
> MTD->_lock/_unlokc
> MTD->_suspend/_resume

Again, that's something that needs to be abstracted so that both the
NAND manufacturer driver and the NAND controller driver can take
appropriate actions on suspend/resume operations.

> NTD->_point/_unpoint

->_point/_unpoint() are irrelevant for a NAND chip.

> ...
> 
> 
> actually, my patch series are including MTD->_locl/_unlock and 
> MTD->_suspend/_resume. how do you think ?

Miquel was suggesting to add nand_chip->{lock,unlock,is_locked}()
methods that would be implemented by the NAND manufacturer drivers, and
have generic wrappers implemented in nand_base.c:

static int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
{
	struct nand_chip *chip = mtd_to_nand(mtd);

	if (!chip->lock)
		return -ENOTSUPP;

	return chip->lock(chip, ofs, len);
}

...

If you do that, you won't need this post_init() hook.

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

Re: [PATCH RFC 2/3] mtd: rawnand: Add support Macronix Block Protection function

[masonccyang]

Hi Boris,

 
> > > > > > Then fill-in these two hooks from the manufacturer code, 
without 
> > the
> > > > > > postponed init.
> > > > > > 
> > > > > 
> > > > > But in the final of nand_scan_tail(), mtd->_lock/_unlock will be
> > > > > filled by NULL, right ? 
> > > > 
> > > > The NAND core should set mtd->_lock/_unlock() to NAND specific 
hooks 
> > so
> > > > that the MTD layer is abstracted and and drivers do not see it. 
Then,
> > > > in the NAND helper, either there is no specific hook defined by a
> > > > manufacturer driver and you return -ENOTSUPP, or you execute the
> > > > defined hook. 
> > > 
> > > okay, patch specific manufacturer _lock/_unlock driver
> > > in nand_manufacturer_init();
> > > 
> > > and in the final of nand_scan_tail()
> > > if (!mtd->_lock)
> > >  mtd->_lock = NULL;
> > > if (!mtd->_unlock)
> > >  mtd->_unlock = NULL; 
> > 
> > 
> > I'm still considering of post_init() in nand_scan_tail() for
> > MTD layer default call-back function replacement because
> > there would be more call-back functions need it.
> > i.e., 
> > MTD->_lock/_unlokc
> > MTD->_suspend/_resume
> 
> Again, that's something that needs to be abstracted so that both the
> NAND manufacturer driver and the NAND controller driver can take
> appropriate actions on suspend/resume operations.
> 
> > NTD->_point/_unpoint
> 
> ->_point/_unpoint() are irrelevant for a NAND chip.
> 
> > ...
> > 
> > 
> > actually, my patch series are including MTD->_locl/_unlock and 
> > MTD->_suspend/_resume. how do you think ?
> 
> Miquel was suggesting to add nand_chip->{lock,unlock,is_locked}()
> methods that would be implemented by the NAND manufacturer drivers, and
> have generic wrappers implemented in nand_base.c:
> 
> static int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> {
>    struct nand_chip *chip = mtd_to_nand(mtd);
> 
>    if (!chip->lock)
>       return -ENOTSUPP;
> 
>    return chip->lock(chip, ofs, len);
> }
> 
> ...
> 
> If you do that, you won't need this post_init() hook.

got it, but ... 
user space program flash_lock/flash_unlock are calling 
mtd_lock() & mtd_unlock().
i.e.,
int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
{
         if (!mtd->_lock)
                 return -EOPNOTSUPP;
         if (ofs < 0 || ofs >= mtd->size || len > mtd->size - ofs)
                 return -EINVAL;
         if (!len)
                 return 0;
         return mtd->_lock(mtd, ofs, len);
}


thanks for your time & comments.
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 RFC 2/3] mtd: rawnand: Add support Macronix Block Protection function

[Boris Brezillon]

On Mon, 21 Oct 2019 16:40:57 +0800
masonccyang@mxic.com.tw wrote:

> Hi Boris,
> 
>  
> > > > > > > Then fill-in these two hooks from the manufacturer code,   
> without 
> > > the  
> > > > > > > postponed init.
> > > > > > >   
> > > > > > 
> > > > > > But in the final of nand_scan_tail(), mtd->_lock/_unlock will be
> > > > > > filled by NULL, right ?   
> > > > > 
> > > > > The NAND core should set mtd->_lock/_unlock() to NAND specific   
> hooks 
> > > so  
> > > > > that the MTD layer is abstracted and and drivers do not see it.   
> Then,
> > > > > in the NAND helper, either there is no specific hook defined by a
> > > > > manufacturer driver and you return -ENOTSUPP, or you execute the
> > > > > defined hook.   
> > > > 
> > > > okay, patch specific manufacturer _lock/_unlock driver
> > > > in nand_manufacturer_init();
> > > > 
> > > > and in the final of nand_scan_tail()
> > > > if (!mtd->_lock)
> > > >  mtd->_lock = NULL;
> > > > if (!mtd->_unlock)
> > > >  mtd->_unlock = NULL;   
> > > 
> > > 
> > > I'm still considering of post_init() in nand_scan_tail() for
> > > MTD layer default call-back function replacement because
> > > there would be more call-back functions need it.
> > > i.e., 
> > > MTD->_lock/_unlokc
> > > MTD->_suspend/_resume  
> > 
> > Again, that's something that needs to be abstracted so that both the
> > NAND manufacturer driver and the NAND controller driver can take
> > appropriate actions on suspend/resume operations.
> >   
> > > NTD->_point/_unpoint  
> >   
> > ->_point/_unpoint() are irrelevant for a NAND chip.  
> >   
> > > ...
> > > 
> > > 
> > > actually, my patch series are including MTD->_locl/_unlock and 
> > > MTD->_suspend/_resume. how do you think ?  
> > 
> > Miquel was suggesting to add nand_chip->{lock,unlock,is_locked}()
> > methods that would be implemented by the NAND manufacturer drivers, and
> > have generic wrappers implemented in nand_base.c:
> > 
> > static int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> > {
> >    struct nand_chip *chip = mtd_to_nand(mtd);
> > 
> >    if (!chip->lock)
> >       return -ENOTSUPP;
> > 
> >    return chip->lock(chip, ofs, len);
> > }
> > 
> > ...
> > 
> > If you do that, you won't need this post_init() hook.  
> 
> got it, but ... 
> user space program flash_lock/flash_unlock are calling 
> mtd_lock() & mtd_unlock().
> i.e.,
> int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> {
>          if (!mtd->_lock)
>                  return -EOPNOTSUPP;
>          if (ofs < 0 || ofs >= mtd->size || len > mtd->size - ofs)
>                  return -EINVAL;
>          if (!len)
>                  return 0;
>          return mtd->_lock(mtd, ofs, len);
> }
> 

Assign mtd lock/unlock/is_locked hooks to the generic wrappers in
nand_scan_tail():

	mtd->_lock = nand_lock;
	mtd->_unlock = nand_unlock;
	mtd->_is_locked = nand_is_locked;

Seriously, we've almost implemented the thing for you with all the
details we've given. At some point you have to look more closely at how
things are done/designed in the NAND framework if you want to
contribute core changes. I'm fine giving hints but we're far beyond
that point here.

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

Re: [PATCH RFC 2/3] mtd: rawnand: Add support Macronix Block Protection function

[masonccyang]

Hi Boris,

> 
> Assign mtd lock/unlock/is_locked hooks to the generic wrappers in
> nand_scan_tail():
> 
>    mtd->_lock = nand_lock;
>    mtd->_unlock = nand_unlock;
>    mtd->_is_locked = nand_is_locked;
> 
> Seriously, we've almost implemented the thing for you with all the
> details we've given. At some point you have to look more closely at how
> things are done/designed in the NAND framework if you want to
> contribute core changes. I'm fine giving hints but we're far beyond
> that point here.

got your point & idea.

thanks a lot for your time & opinions.
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/

[PATCH RFC 3/3] mtd: rawnand: Add support Macronix power down mode

[Mason Yang]

Macronix AD series support using power down command to
enter a minimum power consumption state.

MTD default _suspend/_resume function replacement by
manufacturer postponed initialization.

Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
---
 drivers/mtd/nand/raw/nand_macronix.c | 78 +++++++++++++++++++++++++++++++++++-
 1 file changed, 77 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c
index 991c636..99a7b2e 100644
--- a/drivers/mtd/nand/raw/nand_macronix.c
+++ b/drivers/mtd/nand/raw/nand_macronix.c
@@ -15,6 +15,8 @@
 #define MXIC_BLOCK_PROTECTION_ALL_LOCK 0x38
 #define MXIC_BLOCK_PROTECTION_ALL_UNLOCK 0x0
 
+#define NAND_CMD_POWER_DOWN 0xB9
+
 struct nand_onfi_vendor_macronix {
 	u8 reserved;
 	u8 reliability_func;
@@ -78,6 +80,12 @@ static void macronix_nand_onfi_init(struct nand_chip *chip)
 		"MX30UF4G28AC",
 };
 
+static const char * const nand_power_down[] = {
+		"MX30LF1G28AD",
+		"MX30LF2G28AD",
+		"MX30LF4G28AD",
+};
+
 static void macronix_nand_fix_broken_get_timings(struct nand_chip *chip)
 {
 	unsigned int i;
@@ -144,8 +152,64 @@ static int mxic_nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 	return ret;
 }
 
+int nand_power_down_op(struct nand_chip *chip)
+{
+	int ret;
+
+	if (nand_has_exec_op(chip)) {
+		struct nand_op_instr instrs[] = {
+			NAND_OP_CMD(NAND_CMD_POWER_DOWN, 0),
+		};
+
+		struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs);
+
+		ret = nand_exec_op(chip, &op);
+		if (ret)
+			return ret;
+
+	} else {
+		chip->legacy.cmdfunc(chip, NAND_CMD_POWER_DOWN, -1, -1);
+	}
+
+	return 0;
+}
+
+static int mxic_nand_suspend(struct mtd_info *mtd)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+
+	mutex_lock(&chip->lock);
+
+	nand_select_target(chip, 0);
+	nand_power_down_op(chip);
+	nand_deselect_target(chip);
+
+	chip->suspend = 1;
+	mutex_unlock(&chip->lock);
+
+	return 0;
+}
+
+static void mxic_nand_resume(struct mtd_info *mtd)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+
+	mutex_lock(&chip->lock);
+	// toggle #CS pin to resume NAND device
+	nand_select_target(chip, 0);
+	ndelay(20);
+	nand_deselect_target(chip);
+
+	if (chip->suspend)
+		chip->suspended = 0;
+	else
+		pr_err("%s call for a chip which is not in suspended state\n",
+		       __func__);
+	mutex_unlock(&chip->lock);
+}
+
 /*
- * Macronix AC series support using SET/GET_FEATURES to change
+ * Macronix AC and AD series support using SET/GET_FEATURES to change
  * Block Protection and Unprotection.
  *
  * MTD call-back function replacement by manufacturer postponed
@@ -163,6 +227,18 @@ static void macronix_nand_post_init(struct nand_chip *chip)
 		}
 	}
 
+	for (i = 0; i < ARRAY_SIZE(nand_power_down); i++) {
+		if (!strcmp(nand_power_down[i], chip->parameters.model)) {
+			blockprotected = 1;
+			break;
+		}
+	}
+
+	if (i < ARRAY_SIZE(nand_power_down)) {
+		mtd->_suspend = mxic_nand_suspend;
+		mtd->_resume = mxic_nand_resume;
+	}
+
 	if (blockprotected && chip->parameters.supports_set_get_features) {
 		bitmap_set(chip->parameters.set_feature_list,
 			   ONFI_FEATURE_ADDR_MXIC_PROTECTION, 1);
-- 
1.9.1


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

Re: [PATCH RFC 3/3] mtd: rawnand: Add support Macronix power down mode

[Miquel Raynal]

Hi Mason,

Mason Yang <masonccyang@mxic.com.tw> wrote on Wed, 18 Sep 2019 15:56:26
+0800:

> Macronix AD series support using power down command to
> enter a minimum power consumption state.
> 
> MTD default _suspend/_resume function replacement by
> manufacturer postponed initialization.
> 
> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> ---
>  drivers/mtd/nand/raw/nand_macronix.c | 78 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 77 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c
> index 991c636..99a7b2e 100644
> --- a/drivers/mtd/nand/raw/nand_macronix.c
> +++ b/drivers/mtd/nand/raw/nand_macronix.c
> @@ -15,6 +15,8 @@
>  #define MXIC_BLOCK_PROTECTION_ALL_LOCK 0x38
>  #define MXIC_BLOCK_PROTECTION_ALL_UNLOCK 0x0
>  
> +#define NAND_CMD_POWER_DOWN 0xB9
> +
>  struct nand_onfi_vendor_macronix {
>  	u8 reserved;
>  	u8 reliability_func;
> @@ -78,6 +80,12 @@ static void macronix_nand_onfi_init(struct nand_chip *chip)
>  		"MX30UF4G28AC",
>  };
>  
> +static const char * const nand_power_down[] = {
> +		"MX30LF1G28AD",
> +		"MX30LF2G28AD",
> +		"MX30LF4G28AD",
> +};
> +
>  static void macronix_nand_fix_broken_get_timings(struct nand_chip *chip)
>  {
>  	unsigned int i;
> @@ -144,8 +152,64 @@ static int mxic_nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  	return ret;
>  }
>  
> +int nand_power_down_op(struct nand_chip *chip)
> +{
> +	int ret;
> +
> +	if (nand_has_exec_op(chip)) {
> +		struct nand_op_instr instrs[] = {
> +			NAND_OP_CMD(NAND_CMD_POWER_DOWN, 0),
> +		};
> +
> +		struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs);
> +
> +		ret = nand_exec_op(chip, &op);
> +		if (ret)
> +			return ret;
> +
> +	} else {
> +		chip->legacy.cmdfunc(chip, NAND_CMD_POWER_DOWN, -1, -1);
> +	}
> +
> +	return 0;
> +}
> +
> +static int mxic_nand_suspend(struct mtd_info *mtd)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +
> +	mutex_lock(&chip->lock);
> +
> +	nand_select_target(chip, 0);
> +	nand_power_down_op(chip);
> +	nand_deselect_target(chip);
> +
> +	chip->suspend = 1;
> +	mutex_unlock(&chip->lock);
> +
> +	return 0;
> +}
> +
> +static void mxic_nand_resume(struct mtd_info *mtd)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +
> +	mutex_lock(&chip->lock);
> +	// toggle #CS pin to resume NAND device

C++ style comments are forbidden in code.

> +	nand_select_target(chip, 0);

On several NAND controllers there is no way to act on the CS line
without actually writing bytes to the NAND chip. So basically this
is very likely to not work.

> +	ndelay(20);

Is this delay known somewhere? Is this purely experimental?

> +	nand_deselect_target(chip);
> +
> +	if (chip->suspend)
> +		chip->suspended = 0;
> +	else
> +		pr_err("%s call for a chip which is not in suspended state\n",
> +		       __func__);
> +	mutex_unlock(&chip->lock);
> +}
> +
>  /*
> - * Macronix AC series support using SET/GET_FEATURES to change
> + * Macronix AC and AD series support using SET/GET_FEATURES to change
>   * Block Protection and Unprotection.
>   *
>   * MTD call-back function replacement by manufacturer postponed
> @@ -163,6 +227,18 @@ static void macronix_nand_post_init(struct nand_chip *chip)
>  		}
>  	}
>  
> +	for (i = 0; i < ARRAY_SIZE(nand_power_down); i++) {
> +		if (!strcmp(nand_power_down[i], chip->parameters.model)) {
> +			blockprotected = 1;
> +			break;
> +		}
> +	}
> +
> +	if (i < ARRAY_SIZE(nand_power_down)) {
> +		mtd->_suspend = mxic_nand_suspend;
> +		mtd->_resume = mxic_nand_resume;
> +	}
> +
>  	if (blockprotected && chip->parameters.supports_set_get_features) {
>  		bitmap_set(chip->parameters.set_feature_list,
>  			   ONFI_FEATURE_ADDR_MXIC_PROTECTION, 1);

Thanks,
Miquèl

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

Re: [PATCH RFC 3/3] mtd: rawnand: Add support Macronix power down mode

[masonccyang]

Hi Miquel,


 
> > +int nand_power_down_op(struct nand_chip *chip)
> > +{
> > +   int ret;
> > +
> > +   if (nand_has_exec_op(chip)) {
> > +      struct nand_op_instr instrs[] = {
> > +         NAND_OP_CMD(NAND_CMD_POWER_DOWN, 0),
> > +      };
> > +
> > +      struct nand_operation op = NAND_OPERATION(chip->cur_cs, 
instrs);
> > +
> > +      ret = nand_exec_op(chip, &op);
> > +      if (ret)
> > +         return ret;
> > +
> > +   } else {
> > +      chip->legacy.cmdfunc(chip, NAND_CMD_POWER_DOWN, -1, -1);
> > +   }
> > +
> > +   return 0;
> > +}
> > +
> > +static int mxic_nand_suspend(struct mtd_info *mtd)
> > +{
> > +   struct nand_chip *chip = mtd_to_nand(mtd);
> > +
> > +   mutex_lock(&chip->lock);
> > +
> > +   nand_select_target(chip, 0);
> > +   nand_power_down_op(chip);
> > +   nand_deselect_target(chip);
> > +
> > +   chip->suspend = 1;
> > +   mutex_unlock(&chip->lock);
> > +
> > +   return 0;
> > +}
> > +
> > +static void mxic_nand_resume(struct mtd_info *mtd)
> > +{
> > +   struct nand_chip *chip = mtd_to_nand(mtd);
> > +
> > +   mutex_lock(&chip->lock);
> > +   // toggle #CS pin to resume NAND device
> 
> C++ style comments are forbidden in code.

okay, got it. thanks.

> 
> > +   nand_select_target(chip, 0);
> 
> On several NAND controllers there is no way to act on the CS line
> without actually writing bytes to the NAND chip. So basically this
> is very likely to not work.

any other way to make it work ? GPIO ?
or just have some comments description here.
i.e,.

/* The NAND chip will exit the deep power down mode with #CS toggling, 
 * please refer to datasheet for the timing requirement of tCRDP and tRDP.
 */

> 
> > +   ndelay(20);
> 
> Is this delay known somewhere? Is this purely experimental?

it's timing requirement tCRDP 20 ns(min) to release device
from deep power-down mode. 
You may download datasheet at
https://www.macronix.com/zh-tw/products/NAND-Flash/SLC-NAND-Flash/Pages/spec.aspx?p=MX30LF4G28AD&m=SLC%20NAND&n=PM2579 


> 
> > +   nand_deselect_target(chip);
> > +
> > +   if (chip->suspend)
> > +      chip->suspended = 0;
> > +   else
> > +      pr_err("%s call for a chip which is not in suspended state\n",
> > +             __func__);
> > +   mutex_unlock(&chip->lock);
> > +}

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 RFC 3/3] mtd: rawnand: Add support Macronix power down mode

[Boris Brezillon]

On Tue, 8 Oct 2019 10:06:50 +0800
masonccyang@mxic.com.tw wrote:
   
> > > +   nand_select_target(chip, 0);  
> > 
> > On several NAND controllers there is no way to act on the CS line
> > without actually writing bytes to the NAND chip. So basically this
> > is very likely to not work.  
> 
> any other way to make it work ? GPIO ?
> or just have some comments description here.
> i.e,.
> 
> /* The NAND chip will exit the deep power down mode with #CS toggling, 
>  * please refer to datasheet for the timing requirement of tCRDP and tRDP.
>  */
> 

Good luck with that. As Miquel said, on most NAND controllers
select_target() is a dummy operation that just assigns nand_chip->target
to the specified value but doesn't assert the CS line. You could send a
dummy command here, like a READ_ID, but I guess you need CS to be
asserted for at least 20ns before asserting any other signals (CLE/ALE)
which might be an issue.

> >   
> > > +   ndelay(20);  
> > 
> > Is this delay known somewhere? Is this purely experimental?  
> 
> it's timing requirement tCRDP 20 ns(min) to release device
> from deep power-down mode. 
> You may download datasheet at
> https://www.macronix.com/zh-tw/products/NAND-Flash/SLC-NAND-Flash/Pages/spec.aspx?p=MX30LF4G28AD&m=SLC%20NAND&n=PM2579 

Just looked at the datasheet, and there's actually more than tCRDP:

- you have to make sure you entered power-down state for at least tDPDD
  before you try to wake up the device
- the device goes back to stand-by state tRDP after the CS pin has been
  deasserted.

I guess we can use ndelay() for those, since they happen before/after
the CS pin is asserted/de-asserted. Be careful with ndelay() though,
it's not guaranteed to wait the the time you pass, it can return
before (maybe we should add a helper to deal with that).
Another solution would be to describe CS assertion/de-assertion in
the instruction flow, but that requires patching all exec_op() drivers.

For the tCRDP timing, I think we should use a nand_operation, this way
we can check if the controller is able to deal with dummy CS-assertion
before entering deep-power mode.
In order to do that you'll have to add a NAND_OP_DUMMY_INSTR (or
NAND_OP_DELAY_INSTR), and then have something like:

struct nand_op_instr instrs[] = {
	NAND_OP_DUMMY(tCRDP),
};

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

Re: [PATCH RFC 3/3] mtd: rawnand: Add support Macronix power down mode

[masonccyang]

Hi Boris,

 
> > > > +   nand_select_target(chip, 0); 
> > > 
> > > On several NAND controllers there is no way to act on the CS line
> > > without actually writing bytes to the NAND chip. So basically this
> > > is very likely to not work. 
> > 
> > any other way to make it work ? GPIO ?
> > or just have some comments description here.
> > i.e,.
> > 
> > /* The NAND chip will exit the deep power down mode with #CS toggling, 

> >  * please refer to datasheet for the timing requirement of tCRDP and 
tRDP.
> >  */
> > 
> 
> Good luck with that. As Miquel said, on most NAND controllers
> select_target() is a dummy operation that just assigns nand_chip->target
> to the specified value but doesn't assert the CS line. You could send a
> dummy command here, like a READ_ID, but I guess you need CS to be
> asserted for at least 20ns before asserting any other signals (CLE/ALE)
> which might be an issue.

okay, got it.
But if possible, I think adding CS line control in nand_select_target()
is a simple and generic way for MTD and all raw NAND controllers.

> 
> > > 
> > > > +   ndelay(20); 
> > > 
> > > Is this delay known somewhere? Is this purely experimental? 
> > 
> > it's timing requirement tCRDP 20 ns(min) to release device
> > from deep power-down mode. 
> > You may download datasheet at
> > 
https://www.macronix.com/zh-tw/products/NAND-Flash/SLC-NAND-Flash/Pages/
> spec.aspx?p=MX30LF4G28AD&m=SLC%20NAND&n=PM2579 
> 
> Just looked at the datasheet, and there's actually more than tCRDP:
> 
> - you have to make sure you entered power-down state for at least tDPDD
>   before you try to wake up the device
> - the device goes back to stand-by state tRDP after the CS pin has been
>   deasserted.
> 
> I guess we can use ndelay() for those, since they happen before/after
> the CS pin is asserted/de-asserted. Be careful with ndelay() though,
> it's not guaranteed to wait the the time you pass, it can return
> before (maybe we should add a helper to deal with that).
> Another solution would be to describe CS assertion/de-assertion in
> the instruction flow, but that requires patching all exec_op() drivers.
> 
> For the tCRDP timing, I think we should use a nand_operation, this way
> we can check if the controller is able to deal with dummy CS-assertion
> before entering deep-power mode.
> In order to do that you'll have to add a NAND_OP_DUMMY_INSTR (or
> NAND_OP_DELAY_INSTR), and then have something like:
> 
> struct nand_op_instr instrs[] = {
>    NAND_OP_DUMMY(tCRDP),
> };

got it.

thanks for your time and comments.

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 RFC 3/3] mtd: rawnand: Add support Macronix power down mode

[Miquel Raynal]

Hi Mason,

masonccyang@mxic.com.tw wrote on Tue, 15 Oct 2019 10:33:29 +0800:

> Hi Boris,
> 
>  
> > > > > +   nand_select_target(chip, 0);   
> > > > 
> > > > On several NAND controllers there is no way to act on the CS line
> > > > without actually writing bytes to the NAND chip. So basically this
> > > > is very likely to not work.   
> > > 
> > > any other way to make it work ? GPIO ?
> > > or just have some comments description here.
> > > i.e,.
> > > 
> > > /* The NAND chip will exit the deep power down mode with #CS toggling,   
> 
> > >  * please refer to datasheet for the timing requirement of tCRDP and   
> tRDP.
> > >  */
> > >   
> > 
> > Good luck with that. As Miquel said, on most NAND controllers
> > select_target() is a dummy operation that just assigns nand_chip->target
> > to the specified value but doesn't assert the CS line. You could send a
> > dummy command here, like a READ_ID, but I guess you need CS to be
> > asserted for at least 20ns before asserting any other signals (CLE/ALE)
> > which might be an issue.  
> 
> okay, got it.
> But if possible, I think adding CS line control in nand_select_target()
> is a simple and generic way for MTD and all raw NAND controllers.

The problem is not that we do not want to; the problem is that
controllers are not capable of doing it reliably if no byte is sent
over the NAND bus.


Thanks,
Miquèl

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

Re: [PATCH RFC 3/3] mtd: rawnand: Add support Macronix power down mode

[masonccyang]

Hi Miquel,

> > 
> > > > > > +   nand_select_target(chip, 0); 
> > > > > 
> > > > > On several NAND controllers there is no way to act on the CS 
line
> > > > > without actually writing bytes to the NAND chip. So basically 
this
> > > > > is very likely to not work. 
> > > > 
> > > > any other way to make it work ? GPIO ?
> > > > or just have some comments description here.
> > > > i.e,.
> > > > 
> > > > /* The NAND chip will exit the deep power down mode with #CS 
toggling, 
> > 
> > > >  * please refer to datasheet for the timing requirement of tCRDP 
and 
> > tRDP.
> > > >  */
> > > > 
> > > 
> > > Good luck with that. As Miquel said, on most NAND controllers
> > > select_target() is a dummy operation that just assigns 
nand_chip->target
> > > to the specified value but doesn't assert the CS line. You could 
send a
> > > dummy command here, like a READ_ID, but I guess you need CS to be
> > > asserted for at least 20ns before asserting any other signals 
(CLE/ALE)
> > > which might be an issue. 
> > 
> > okay, got it.
> > But if possible, I think adding CS line control in 
nand_select_target()
> > is a simple and generic way for MTD and all raw NAND controllers.
> 
> The problem is not that we do not want to; the problem is that
> controllers are not capable of doing it reliably if no byte is sent
> over the NAND bus.

okay,  it's kind of pity even though our raw NAND controller is capable of 

doing it with no byte is sent over the NAND bus.

As you mentioned that other controllers are not capable of doing it 
reliably 
if no byte is sent over the NAND bus.
if so, does it work by adding a NAND_OP_DUMMY_INSTR ? (as Boris's 
comments)

thanks for your time & comments.

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 RFC 3/3] mtd: rawnand: Add support Macronix power down mode

[masonccyang]

Hi Boris,

> 
> Subject
> 
> Re: [PATCH RFC 3/3] mtd: rawnand: Add support Macronix power down mode
> 
> On Tue, 8 Oct 2019 10:06:50 +0800
> masonccyang@mxic.com.tw wrote:
> 
> > > > +   nand_select_target(chip, 0); 
> > > 
> > > On several NAND controllers there is no way to act on the CS line
> > > without actually writing bytes to the NAND chip. So basically this
> > > is very likely to not work. 
> > 
> > any other way to make it work ? GPIO ?
> > or just have some comments description here.
> > i.e,.
> > 
> > /* The NAND chip will exit the deep power down mode with #CS toggling, 

> >  * please refer to datasheet for the timing requirement of tCRDP and 
tRDP.
> >  */
> > 
> 
> Good luck with that. As Miquel said, on most NAND controllers
> select_target() is a dummy operation that just assigns nand_chip->target
> to the specified value but doesn't assert the CS line. You could send a
> dummy command here, like a READ_ID, but I guess you need CS to be
> asserted for at least 20ns before asserting any other signals (CLE/ALE)
> which might be an issue.
> 

I have validated & checked w/ designer that NANDs device just need CS# 
line
toggling and don't care of CLE/WE#/RE#. That is raw NAND controller 
sending
a command nand_status_op() or nand_power_down_op() will exit the deep 
power down too. The first command(just CS# line toggling) to NAND device 
in deep power down mode will not be recognized. We may publish the 
application notes or update the datasheet later.

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/