[PATCH 0/6] Improve timings handling in the NAND framework

[PATCH 0/6] Improve timings handling in the NAND framework

[Miquel Raynal]

Hello,

This series evolves how GET/SET_FEATURES operations are handled by the
NAND core by adding two helpers that should be called instead of calling
directly the ->onfi_get/set_feature() hooks.

Then, the parameter page that was always allocated no matter if the NAND
core could make use of it is dropped. Instead, a much smaller structure
is embedded in the nand_chip structure and just stores the information
that will be useful for the core. Future enhancements of the core that
will need more information will just have to add a new entry in this
structure and fill it during the detection phase. A bitmap is used to
know which optional feature is actually supported.

This give the possibility in vendor code to overload this bitmap and
force the addition/removal of supported features in the list, depending
for instance on the NAND chip currently being used.

This new possibility is effectively used for Macronix NAND chip
MX30LF2G18AC. This chip supports natively the timing mode 5, and its
parameter page indicates that it supports GET/SET_FEATURES on timing
modes, while in reality it does not. Removing this feature from the
supported bitmap makes it usable at high speed instead of unnecessarily
limiting it to timing mode 0.

Thank you,
Miquèl

Miquel Raynal (6):
  mtd: nand: Avoid setting again the timings to mode 0 after a reset
  mtd: nand: Use wrappers to call onfi GET/SET_FEATURES
  mtd: nand: mxc: Remove useless checks in GET/SET_FEATURES functions
  mtd: nand: Stop using a static parameter page for all chips
  mtd: nand: Allow vendors to declare (un)supported features
  mtd: nand: macronix: Unflag the support of changing timings for
    MX30LF2G18AC

 drivers/mtd/nand/mxc_nand.c      |  10 --
 drivers/mtd/nand/nand_base.c     | 259 ++++++++++++++++++++++++++-------------
 drivers/mtd/nand/nand_macronix.c |  12 ++
 drivers/mtd/nand/nand_micron.c   |  35 +++---
 drivers/mtd/nand/nand_timings.c  |  10 +-
 include/linux/mtd/rawnand.h      |  53 ++++----
 6 files changed, 231 insertions(+), 148 deletions(-)

-- 
2.14.1

[PATCH 1/6] mtd: nand: Avoid setting again the timings to mode 0 after a reset

[Miquel Raynal]

After a nand_reset_data_interface(), both the NAND chip and the NAND
controller use timing mode 0. The previously defined data interface for
this chip has been saved and is supposed to be restored after that.
However, if the saved data interface also refers to timing mode 0, there
is no need to re-apply them again.

Also, as nand_setup_data_interface() uses ->set/get_features(), it could
lead to issues when doing the reset at probe time as the parameter page
is not available yet to know if these functions are supported or not.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/nand_base.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 9602d17845e2..00111e669c11 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2771,6 +2771,16 @@ int nand_reset(struct nand_chip *chip, int chipnr)
 	if (ret)
 		return ret;
 
+	/*
+	 * A nand_reset_data_interface() put both the NAND chip and the NAND
+	 * controller in timings mode 0. If the default mode for this chip is
+	 * also 0, no need to proceed to the change again. Plus, at probe time,
+	 * nand_setup_data_interface() uses ->set/get_features() which would
+	 * fail anyway as the parameter page is not available yet.
+	 */
+	if (!chip->onfi_timing_mode_default)
+		return 0;
+
 	chip->data_interface = saved_data_intf;
 	ret = nand_setup_data_interface(chip, chipnr);
 	if (ret)
-- 
2.14.1

[PATCH 2/6] mtd: nand: Use wrappers to call onfi GET/SET_FEATURES

[Miquel Raynal]

From: Miquel Raynal <miquel.raynal@free-electrons.com>

Prepare the fact that some features managed by GET/SET_FEATURES could be
overloaded by vendor code. To handle this logic, use new wrappers
instead of directly call the ->onfi_get/set_features() hooks.

Also take into account that not having the proper SET/GET_FEATURES
available is not a reason to return an error. The wrappers that are
created here handle this case by returning a special error code:
-ENOTSUPP. More logic in the calling function of the new helpers
(nand_setup_data_interface()) is added to handle this case).

Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 drivers/mtd/nand/nand_base.c   | 145 ++++++++++++++++++++++++++---------------
 drivers/mtd/nand/nand_micron.c |  16 ++---
 include/linux/mtd/rawnand.h    |   3 +
 3 files changed, 104 insertions(+), 60 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 00111e669c11..8d2c37011979 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1160,6 +1160,52 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
 	return status;
 }
 
+/**
+ * nand_get_features - wrapper to perform a GET_FEATURE
+ * @chip: NAND chip info structure
+ * @addr: feature address
+ * @subfeature_param: the subfeature parameters, a four bytes array
+ *
+ * Returns 0 for success, a negative error otherwise. Returns -ENOTSUPP if the
+ * operation cannot be handled.
+ */
+int nand_get_features(struct nand_chip *chip, int addr,
+		      u8 *subfeature_param)
+{
+	struct mtd_info *mtd = nand_to_mtd(chip);
+
+	if (!chip->onfi_version ||
+	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
+	      & ONFI_OPT_CMD_SET_GET_FEATURES))
+		return -ENOTSUPP;
+
+	return chip->onfi_get_features(mtd, chip, addr, subfeature_param);
+}
+EXPORT_SYMBOL_GPL(nand_get_features);
+
+/**
+ * nand_set_features - wrapper to perform a SET_FEATURE
+ * @chip: NAND chip info structure
+ * @addr: feature address
+ * @subfeature_param: the subfeature parameters, a four bytes array
+ *
+ * Returns 0 for success, a negative error otherwise. Returns -ENOTSUPP if the
+ * operation cannot be handled.
+ */
+int nand_set_features(struct nand_chip *chip, int addr,
+		      u8 *subfeature_param)
+{
+	struct mtd_info *mtd = nand_to_mtd(chip);
+
+	if (!chip->onfi_version ||
+	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
+	      & ONFI_OPT_CMD_SET_GET_FEATURES))
+		return -ENOTSUPP;
+
+	return chip->onfi_set_features(mtd, chip, addr, subfeature_param);
+}
+EXPORT_SYMBOL_GPL(nand_set_features);
+
 /**
  * nand_reset_data_interface - Reset data interface and timings
  * @chip: The NAND chip
@@ -1215,59 +1261,62 @@ static int nand_reset_data_interface(struct nand_chip *chip, int chipnr)
 static int nand_setup_data_interface(struct nand_chip *chip, int chipnr)
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
+	u8 tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = {
+		chip->onfi_timing_mode_default,
+	};
 	int ret;
 
 	if (!chip->setup_data_interface)
 		return 0;
 
+	/* Change the mode on the chip side */
+	chip->select_chip(mtd, chipnr);
+	ret = nand_set_features(chip, ONFI_FEATURE_ADDR_TIMING_MODE,
+				tmode_param);
+	chip->select_chip(mtd, -1);
+
 	/*
-	 * Ensure the timing mode has been changed on the chip side
-	 * before changing timings on the controller side.
+	 * Do not fail because the mode cannot explicitly be set. If the NAND
+	 * chip claims it supports it, let's just apply the timings on the
+	 * controller side.
 	 */
-	if (chip->onfi_version &&
-	    (le16_to_cpu(chip->onfi_params.opt_cmd) &
-	     ONFI_OPT_CMD_SET_GET_FEATURES)) {
-		u8 tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = {
-			chip->onfi_timing_mode_default,
-		};
-
-		chip->select_chip(mtd, chipnr);
-		ret = chip->onfi_set_features(mtd, chip,
-				ONFI_FEATURE_ADDR_TIMING_MODE,
-				tmode_param);
-		chip->select_chip(mtd, -1);
-		if (ret)
-			return ret;
-	}
+	if (ret && ret != -ENOTSUPP)
+		return ret;
 
+	/* Change the mode on the controller side */
 	ret = chip->setup_data_interface(mtd, chipnr, &chip->data_interface);
 	if (ret)
 		return ret;
 
-	if (chip->onfi_version &&
-	    (le16_to_cpu(chip->onfi_params.opt_cmd) &
-	     ONFI_OPT_CMD_SET_GET_FEATURES)) {
-		u8 tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = {};
+	/* Check the mode has been accepted by the chip */
+	memset(tmode_param, 0, ONFI_SUBFEATURE_PARAM_LEN);
+	chip->select_chip(mtd, chipnr);
+	ret = nand_get_features(chip, ONFI_FEATURE_ADDR_TIMING_MODE,
+				tmode_param);
+	chip->select_chip(mtd, -1);
+	if (ret && ret != -ENOTSUPP)
+		goto err_reset_chip;
 
-		chip->select_chip(mtd, chipnr);
-		ret = chip->onfi_get_features(mtd, chip,
-					      ONFI_FEATURE_ADDR_TIMING_MODE,
-					      tmode_param);
-		chip->select_chip(mtd, -1);
-		if (ret)
-			goto err_reset_chip;
+	/*
+	 * Do not fail because the mode could not be checked. However, skip the
+	 * comparison block that would probably raise an error.
+	 */
+	if (ret == -ENOTSUPP)
+		return 0;
 
-		if (tmode_param[0] != chip->onfi_timing_mode_default) {
-			pr_warn("timings mode %d not acknowledged by the NAND chip\n",
-				chip->onfi_timing_mode_default);
-			goto err_reset_chip;
-		}
+	if (tmode_param[0] != chip->onfi_timing_mode_default) {
+		pr_warn("timing mode %d not acknowledged by the NAND chip\n",
+			chip->onfi_timing_mode_default);
+		goto err_reset_chip;
 	}
 
 	return 0;
 
 err_reset_chip:
-	/* Fallback to timing mode 0 */
+	/*
+	 * Fallback to mode 0 if the chip explicitly did not ack the chosen
+	 * timing mode.
+	 */
 	nand_reset_data_interface(chip, chipnr);
 	chip->select_chip(mtd, chipnr);
 	nand_reset_op(chip);
@@ -4905,38 +4954,30 @@ static int nand_max_bad_blocks(struct mtd_info *mtd, loff_t ofs, size_t len)
 }
 
 /**
- * nand_onfi_set_features- [REPLACEABLE] set features for ONFI nand
+ * nand_set_features_default - [REPLACEABLE] set features for ONFI NAND
  * @mtd: MTD device structure
  * @chip: nand chip info structure
  * @addr: feature address.
  * @subfeature_param: the subfeature parameters, a four bytes array.
  */
-static int nand_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
-			int addr, uint8_t *subfeature_param)
+static int nand_set_features_default(struct mtd_info *mtd,
+				     struct nand_chip *chip, int addr,
+				     uint8_t *subfeature_param)
 {
-	if (!chip->onfi_version ||
-	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
-	      & ONFI_OPT_CMD_SET_GET_FEATURES))
-		return -EINVAL;
-
 	return nand_set_features_op(chip, addr, subfeature_param);
 }
 
 /**
- * nand_onfi_get_features- [REPLACEABLE] get features for ONFI nand
+ * nand_get_features_default - [REPLACEABLE] get features for ONFI NAND
  * @mtd: MTD device structure
  * @chip: nand chip info structure
  * @addr: feature address.
  * @subfeature_param: the subfeature parameters, a four bytes array.
  */
-static int nand_onfi_get_features(struct mtd_info *mtd, struct nand_chip *chip,
-			int addr, uint8_t *subfeature_param)
+static int nand_get_features_default(struct mtd_info *mtd,
+				     struct nand_chip *chip, int addr,
+				     uint8_t *subfeature_param)
 {
-	if (!chip->onfi_version ||
-	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
-	      & ONFI_OPT_CMD_SET_GET_FEATURES))
-		return -EINVAL;
-
 	return nand_get_features_op(chip, addr, subfeature_param);
 }
 
@@ -5015,9 +5056,9 @@ static void nand_set_defaults(struct nand_chip *chip)
 
 	/* set for ONFI nand */
 	if (!chip->onfi_set_features)
-		chip->onfi_set_features = nand_onfi_set_features;
+		chip->onfi_set_features = nand_set_features_default;
 	if (!chip->onfi_get_features)
-		chip->onfi_get_features = nand_onfi_get_features;
+		chip->onfi_get_features = nand_get_features_default;
 
 	/* If called twice, pointers that depend on busw may need to be reset */
 	if (!chip->read_byte || chip->read_byte == nand_read_byte)
diff --git a/drivers/mtd/nand/nand_micron.c b/drivers/mtd/nand/nand_micron.c
index 02e109ae73f1..48847b441ef7 100644
--- a/drivers/mtd/nand/nand_micron.c
+++ b/drivers/mtd/nand/nand_micron.c
@@ -48,8 +48,7 @@ static int micron_nand_setup_read_retry(struct mtd_info *mtd, int retry_mode)
 	struct nand_chip *chip = mtd_to_nand(mtd);
 	u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = {retry_mode};
 
-	return chip->onfi_set_features(mtd, chip, ONFI_FEATURE_ADDR_READ_RETRY,
-				       feature);
+	return nand_set_features(chip, ONFI_FEATURE_ADDR_READ_RETRY, feature);
 }
 
 /*
@@ -108,8 +107,7 @@ static int micron_nand_on_die_ecc_setup(struct nand_chip *chip, bool enable)
 	if (enable)
 		feature[0] |= ONFI_FEATURE_ON_DIE_ECC_EN;
 
-	return chip->onfi_set_features(nand_to_mtd(chip), chip,
-				       ONFI_FEATURE_ON_DIE_ECC, feature);
+	return nand_set_features(chip, ONFI_FEATURE_ON_DIE_ECC, feature);
 }
 
 static int
@@ -219,8 +217,9 @@ static int micron_supports_on_die_ecc(struct nand_chip *chip)
 	if (ret)
 		return MICRON_ON_DIE_UNSUPPORTED;
 
-	chip->onfi_get_features(nand_to_mtd(chip), chip,
-				ONFI_FEATURE_ON_DIE_ECC, feature);
+	ret = nand_get_features(chip, ONFI_FEATURE_ON_DIE_ECC, feature);
+	if (ret < 0)
+		return ret;
 	if ((feature[0] & ONFI_FEATURE_ON_DIE_ECC_EN) == 0)
 		return MICRON_ON_DIE_UNSUPPORTED;
 
@@ -228,8 +227,9 @@ static int micron_supports_on_die_ecc(struct nand_chip *chip)
 	if (ret)
 		return MICRON_ON_DIE_UNSUPPORTED;
 
-	chip->onfi_get_features(nand_to_mtd(chip), chip,
-				ONFI_FEATURE_ON_DIE_ECC, feature);
+	ret = nand_get_features(chip, ONFI_FEATURE_ON_DIE_ECC, feature);
+	if (ret < 0)
+		return ret;
 	if (feature[0] & ONFI_FEATURE_ON_DIE_ECC_EN)
 		return MICRON_ON_DIE_MANDATORY;
 
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index 469dc724f5df..3244f2594b6b 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -1629,6 +1629,9 @@ int nand_read_oob_std(struct mtd_info *mtd, struct nand_chip *chip, int page);
 int nand_read_oob_syndrome(struct mtd_info *mtd, struct nand_chip *chip,
 			   int page);
 
+/* Wrapper to use in order for controllers/vendors to GET/SET FEATURES */
+int nand_get_features(struct nand_chip *chip, int addr, u8 *subfeature_param);
+int nand_set_features(struct nand_chip *chip, int addr, u8 *subfeature_param);
 /* Stub used by drivers that do not support GET/SET FEATURES operations */
 int nand_onfi_get_set_features_notsupp(struct mtd_info *mtd,
 				       struct nand_chip *chip, int addr,
-- 
2.14.1

Re: [PATCH 2/6] mtd: nand: Use wrappers to call onfi GET/SET_FEATURES

[Boris Brezillon]

Hi Miquel,

On Sat,  3 Feb 2018 10:55:40 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> From: Miquel Raynal <miquel.raynal@free-electrons.com>
> 
> Prepare the fact that some features managed by GET/SET_FEATURES could be
> overloaded by vendor code. To handle this logic, use new wrappers
> instead of directly call the ->onfi_get/set_features() hooks.
> 
> Also take into account that not having the proper SET/GET_FEATURES
> available is not a reason to return an error. The wrappers that are
> created here handle this case by returning a special error code:
> -ENOTSUPP. More logic in the calling function of the new helpers
> (nand_setup_data_interface()) is added to handle this case).
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> ---
>  drivers/mtd/nand/nand_base.c   | 145 ++++++++++++++++++++++++++---------------
>  drivers/mtd/nand/nand_micron.c |  16 ++---
>  include/linux/mtd/rawnand.h    |   3 +
>  3 files changed, 104 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 00111e669c11..8d2c37011979 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -1160,6 +1160,52 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
>  	return status;
>  }
>  
> +/**
> + * nand_get_features - wrapper to perform a GET_FEATURE
> + * @chip: NAND chip info structure
> + * @addr: feature address
> + * @subfeature_param: the subfeature parameters, a four bytes array
> + *
> + * Returns 0 for success, a negative error otherwise. Returns -ENOTSUPP if the
> + * operation cannot be handled.
> + */
> +int nand_get_features(struct nand_chip *chip, int addr,
> +		      u8 *subfeature_param)
> +{
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +
> +	if (!chip->onfi_version ||
> +	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
> +	      & ONFI_OPT_CMD_SET_GET_FEATURES))
> +		return -ENOTSUPP;
> +
> +	return chip->onfi_get_features(mtd, chip, addr, subfeature_param);
> +}
> +EXPORT_SYMBOL_GPL(nand_get_features);
> +
> +/**
> + * nand_set_features - wrapper to perform a SET_FEATURE
> + * @chip: NAND chip info structure
> + * @addr: feature address
> + * @subfeature_param: the subfeature parameters, a four bytes array
> + *
> + * Returns 0 for success, a negative error otherwise. Returns -ENOTSUPP if the
> + * operation cannot be handled.
> + */
> +int nand_set_features(struct nand_chip *chip, int addr,
> +		      u8 *subfeature_param)
> +{
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +
> +	if (!chip->onfi_version ||
> +	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
> +	      & ONFI_OPT_CMD_SET_GET_FEATURES))
> +		return -ENOTSUPP;
> +
> +	return chip->onfi_set_features(mtd, chip, addr, subfeature_param);
> +}
> +EXPORT_SYMBOL_GPL(nand_set_features);
> +
>  /**
>   * nand_reset_data_interface - Reset data interface and timings
>   * @chip: The NAND chip
> @@ -1215,59 +1261,62 @@ static int nand_reset_data_interface(struct nand_chip *chip, int chipnr)
>  static int nand_setup_data_interface(struct nand_chip *chip, int chipnr)
>  {
>  	struct mtd_info *mtd = nand_to_mtd(chip);
> +	u8 tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = {
> +		chip->onfi_timing_mode_default,
> +	};
>  	int ret;
>  
>  	if (!chip->setup_data_interface)
>  		return 0;
>  
> +	/* Change the mode on the chip side */
> +	chip->select_chip(mtd, chipnr);
> +	ret = nand_set_features(chip, ONFI_FEATURE_ADDR_TIMING_MODE,
> +				tmode_param);
> +	chip->select_chip(mtd, -1);
> +
>  	/*
> -	 * Ensure the timing mode has been changed on the chip side
> -	 * before changing timings on the controller side.
> +	 * Do not fail because the mode cannot explicitly be set. If the NAND
> +	 * chip claims it supports it, let's just apply the timings on the
> +	 * controller side.
>  	 */
> -	if (chip->onfi_version &&
> -	    (le16_to_cpu(chip->onfi_params.opt_cmd) &
> -	     ONFI_OPT_CMD_SET_GET_FEATURES)) {
> -		u8 tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = {
> -			chip->onfi_timing_mode_default,
> -		};
> -
> -		chip->select_chip(mtd, chipnr);
> -		ret = chip->onfi_set_features(mtd, chip,
> -				ONFI_FEATURE_ADDR_TIMING_MODE,
> -				tmode_param);
> -		chip->select_chip(mtd, -1);
> -		if (ret)
> -			return ret;
> -	}
> +	if (ret && ret != -ENOTSUPP)
> +		return ret;
>  
> +	/* Change the mode on the controller side */
>  	ret = chip->setup_data_interface(mtd, chipnr, &chip->data_interface);
>  	if (ret)
>  		return ret;
>  
> -	if (chip->onfi_version &&
> -	    (le16_to_cpu(chip->onfi_params.opt_cmd) &
> -	     ONFI_OPT_CMD_SET_GET_FEATURES)) {
> -		u8 tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = {};
> +	/* Check the mode has been accepted by the chip */
> +	memset(tmode_param, 0, ONFI_SUBFEATURE_PARAM_LEN);
> +	chip->select_chip(mtd, chipnr);
> +	ret = nand_get_features(chip, ONFI_FEATURE_ADDR_TIMING_MODE,
> +				tmode_param);
> +	chip->select_chip(mtd, -1);
> +	if (ret && ret != -ENOTSUPP)
> +		goto err_reset_chip;
>  
> -		chip->select_chip(mtd, chipnr);
> -		ret = chip->onfi_get_features(mtd, chip,
> -					      ONFI_FEATURE_ADDR_TIMING_MODE,
> -					      tmode_param);
> -		chip->select_chip(mtd, -1);
> -		if (ret)
> -			goto err_reset_chip;
> +	/*
> +	 * Do not fail because the mode could not be checked. However, skip the
> +	 * comparison block that would probably raise an error.
> +	 */
> +	if (ret == -ENOTSUPP)
> +		return 0;
>  
> -		if (tmode_param[0] != chip->onfi_timing_mode_default) {
> -			pr_warn("timings mode %d not acknowledged by the NAND chip\n",
> -				chip->onfi_timing_mode_default);
> -			goto err_reset_chip;
> -		}
> +	if (tmode_param[0] != chip->onfi_timing_mode_default) {
> +		pr_warn("timing mode %d not acknowledged by the NAND chip\n",
> +			chip->onfi_timing_mode_default);
> +		goto err_reset_chip;
>  	}
>  
>  	return 0;
>  
>  err_reset_chip:
> -	/* Fallback to timing mode 0 */
> +	/*
> +	 * Fallback to mode 0 if the chip explicitly did not ack the chosen
> +	 * timing mode.
> +	 */
>  	nand_reset_data_interface(chip, chipnr);
>  	chip->select_chip(mtd, chipnr);
>  	nand_reset_op(chip);
> @@ -4905,38 +4954,30 @@ static int nand_max_bad_blocks(struct mtd_info *mtd, loff_t ofs, size_t len)
>  }
>  
>  /**
> - * nand_onfi_set_features- [REPLACEABLE] set features for ONFI nand
> + * nand_set_features_default - [REPLACEABLE] set features for ONFI NAND
>   * @mtd: MTD device structure
>   * @chip: nand chip info structure
>   * @addr: feature address.
>   * @subfeature_param: the subfeature parameters, a four bytes array.
>   */
> -static int nand_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
> -			int addr, uint8_t *subfeature_param)
> +static int nand_set_features_default(struct mtd_info *mtd,
> +				     struct nand_chip *chip, int addr,
> +				     uint8_t *subfeature_param)

This name change is not mentioned in the commit message, and it's
probably something you should do in a separate patch. And how about
moving the default specifier just after nand_ =>
nand_default_set_features().

>  {
> -	if (!chip->onfi_version ||
> -	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
> -	      & ONFI_OPT_CMD_SET_GET_FEATURES))
> -		return -EINVAL;
> -
>  	return nand_set_features_op(chip, addr, subfeature_param);
>  }
>  
>  /**
> - * nand_onfi_get_features- [REPLACEABLE] get features for ONFI nand
> + * nand_get_features_default - [REPLACEABLE] get features for ONFI NAND
>   * @mtd: MTD device structure
>   * @chip: nand chip info structure
>   * @addr: feature address.
>   * @subfeature_param: the subfeature parameters, a four bytes array.
>   */
> -static int nand_onfi_get_features(struct mtd_info *mtd, struct nand_chip *chip,
> -			int addr, uint8_t *subfeature_param)
> +static int nand_get_features_default(struct mtd_info *mtd,
> +				     struct nand_chip *chip, int addr,
> +				     uint8_t *subfeature_param)

Ditto.

>  {
> -	if (!chip->onfi_version ||
> -	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
> -	      & ONFI_OPT_CMD_SET_GET_FEATURES))
> -		return -EINVAL;
> -
>  	return nand_get_features_op(chip, addr, subfeature_param);
>  }
>  
> @@ -5015,9 +5056,9 @@ static void nand_set_defaults(struct nand_chip *chip)
>  
>  	/* set for ONFI nand */
>  	if (!chip->onfi_set_features)
> -		chip->onfi_set_features = nand_onfi_set_features;
> +		chip->onfi_set_features = nand_set_features_default;
>  	if (!chip->onfi_get_features)
> -		chip->onfi_get_features = nand_onfi_get_features;
> +		chip->onfi_get_features = nand_get_features_default;

We should probably also rename the hooks at some point, because GET/SET
FEATURES operations are not limited to ONFi compliant chips.

Thanks,

Boris

-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH 2/6] mtd: nand: Use wrappers to call onfi GET/SET_FEATURES

[Miquel Raynal]

Hi Boris,

On Tue, 6 Feb 2018 15:55:08 +0100, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:

> On Sat,  3 Feb 2018 10:55:40 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> >  /**
> > - * nand_onfi_set_features- [REPLACEABLE] set features for ONFI nand
> > + * nand_set_features_default - [REPLACEABLE] set features for ONFI NAND
> >   * @mtd: MTD device structure
> >   * @chip: nand chip info structure
> >   * @addr: feature address.
> >   * @subfeature_param: the subfeature parameters, a four bytes array.
> >   */
> > -static int nand_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
> > -			int addr, uint8_t *subfeature_param)
> > +static int nand_set_features_default(struct mtd_info *mtd,
> > +				     struct nand_chip *chip, int addr,
> > +				     uint8_t *subfeature_param)  
> 
> This name change is not mentioned in the commit message, and it's
> probably something you should do in a separate patch. And how about
> moving the default specifier just after nand_ =>
> nand_default_set_features().

This change has been moved in a separate patch, that will be integrated
in the series that prepares to this one (about a better handling of
timings in the core).

> 
> >  {
> > -	if (!chip->onfi_version ||
> > -	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
> > -	      & ONFI_OPT_CMD_SET_GET_FEATURES))
> > -		return -EINVAL;
> > -
> >  	return nand_set_features_op(chip, addr, subfeature_param);
> >  }
> >  
> >  /**
> > - * nand_onfi_get_features- [REPLACEABLE] get features for ONFI nand
> > + * nand_get_features_default - [REPLACEABLE] get features for ONFI NAND
> >   * @mtd: MTD device structure
> >   * @chip: nand chip info structure
> >   * @addr: feature address.
> >   * @subfeature_param: the subfeature parameters, a four bytes array.
> >   */
> > -static int nand_onfi_get_features(struct mtd_info *mtd, struct nand_chip *chip,
> > -			int addr, uint8_t *subfeature_param)
> > +static int nand_get_features_default(struct mtd_info *mtd,
> > +				     struct nand_chip *chip, int addr,
> > +				     uint8_t *subfeature_param)  
> 
> Ditto.
> 
> >  {
> > -	if (!chip->onfi_version ||
> > -	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
> > -	      & ONFI_OPT_CMD_SET_GET_FEATURES))
> > -		return -EINVAL;
> > -
> >  	return nand_get_features_op(chip, addr, subfeature_param);
> >  }
> >  
> > @@ -5015,9 +5056,9 @@ static void nand_set_defaults(struct nand_chip *chip)
> >  
> >  	/* set for ONFI nand */
> >  	if (!chip->onfi_set_features)
> > -		chip->onfi_set_features = nand_onfi_set_features;
> > +		chip->onfi_set_features = nand_set_features_default;
> >  	if (!chip->onfi_get_features)
> > -		chip->onfi_get_features = nand_onfi_get_features;
> > +		chip->onfi_get_features = nand_get_features_default;  
> 
> We should probably also rename the hooks at some point, because GET/SET
> FEATURES operations are not limited to ONFi compliant chips.

Done. Will also be in the other series. Next version is coming soon.

Thanks,
Miquèl

-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

[PATCH 3/6] mtd: nand: mxc: Remove useless checks in GET/SET_FEATURES functions

[Miquel Raynal]

From: Miquel Raynal <miquel.raynal@free-electrons.com>

All the calls to the chip's hooks ->onfi_get/set_features() go through
the core's wrappers nand_get/set_features() that already do the
necessary checks about ONFI compliance and feature support. Remove these
checks from the mxc's functions.

Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 drivers/mtd/nand/mxc_nand.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index f3be0b2a8869..e36f6ae50bdd 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -1324,11 +1324,6 @@ static int mxc_nand_onfi_set_features(struct mtd_info *mtd,
 	struct mxc_nand_host *host = nand_get_controller_data(nand_chip);
 	int i;
 
-	if (!chip->onfi_version ||
-	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
-	      & ONFI_OPT_CMD_SET_GET_FEATURES))
-		return -EINVAL;
-
 	host->buf_start = 0;
 
 	for (i = 0; i < ONFI_SUBFEATURE_PARAM_LEN; ++i)
@@ -1350,11 +1345,6 @@ static int mxc_nand_onfi_get_features(struct mtd_info *mtd,
 	struct mxc_nand_host *host = nand_get_controller_data(nand_chip);
 	int i;
 
-	if (!chip->onfi_version ||
-	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
-	      & ONFI_OPT_CMD_SET_GET_FEATURES))
-		return -EINVAL;
-
 	host->devtype_data->send_cmd(host, NAND_CMD_GET_FEATURES, false);
 	mxc_do_addr_cycle(mtd, addr, -1);
 	host->devtype_data->send_page(mtd, NFC_OUTPUT);
-- 
2.14.1

[PATCH 4/6] mtd: nand: Stop using a static parameter page for all chips

[Miquel Raynal]

From: Miquel Raynal <miquel.raynal@free-electrons.com>

The NAND chip parameter page is statically allocated within the
nand_chip structure, which reserves a lot of space. Even not ONFI nor
JEDEC chips have it embedded. Also, only a few parameters are still be
read from the parameter page after the detection.

Remove these pages from the nand_chip structure and instead create a
small structure with all the parameters that will be needed outside of
the identification phase.

During identification, allocate dynamically the parameter page after the
chip is declared ONFI or JEDEC. Extract all information from it and free
this space when exiting.

Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 drivers/mtd/nand/nand_base.c    | 111 +++++++++++++++++++++++++---------------
 drivers/mtd/nand/nand_micron.c  |  15 +++---
 drivers/mtd/nand/nand_timings.c |  10 ++--
 include/linux/mtd/rawnand.h     |  45 ++++++----------
 4 files changed, 98 insertions(+), 83 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 8d2c37011979..4a879b1635b3 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1174,9 +1174,7 @@ int nand_get_features(struct nand_chip *chip, int addr,
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
 
-	if (!chip->onfi_version ||
-	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
-	      & ONFI_OPT_CMD_SET_GET_FEATURES))
+	if (!chip->parameters.support_setting_features)
 		return -ENOTSUPP;
 
 	return chip->onfi_get_features(mtd, chip, addr, subfeature_param);
@@ -1197,9 +1195,7 @@ int nand_set_features(struct nand_chip *chip, int addr,
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
 
-	if (!chip->onfi_version ||
-	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
-	      & ONFI_OPT_CMD_SET_GET_FEATURES))
+	if (!chip->parameters.support_setting_features)
 		return -ENOTSUPP;
 
 	return chip->onfi_set_features(mtd, chip, addr, subfeature_param);
@@ -5198,7 +5194,7 @@ static int nand_flash_detect_ext_param_page(struct nand_chip *chip,
 static int nand_flash_detect_onfi(struct nand_chip *chip)
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
-	struct nand_onfi_params *p = &chip->onfi_params;
+	struct nand_onfi_params *p;
 	char id[4];
 	int i, ret, val;
 
@@ -5207,14 +5203,23 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
 	if (ret || strncmp(id, "ONFI", 4))
 		return 0;
 
+	/* ONFI chip: allocate a buffer to hold its parameter page */
+	p = kzalloc(sizeof(*p), GFP_KERNEL);
+	if (!p)
+		return -ENOMEM;
+
 	ret = nand_read_param_page_op(chip, 0, NULL, 0);
-	if (ret)
-		return 0;
+	if (ret) {
+		ret = 0;
+		goto free_onfi_param_page;
+	}
 
 	for (i = 0; i < 3; i++) {
 		ret = nand_read_data_op(chip, p, sizeof(*p), true);
-		if (ret)
-			return 0;
+		if (ret) {
+			ret = 0;
+			goto free_onfi_param_page;
+		}
 
 		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
 				le16_to_cpu(p->crc)) {
@@ -5224,7 +5229,7 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
 
 	if (i == 3) {
 		pr_err("Could not find valid ONFI parameter page; aborting\n");
-		return 0;
+		goto free_onfi_param_page;
 	}
 
 	/* Check version */
@@ -5242,13 +5247,16 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
 
 	if (!chip->onfi_version) {
 		pr_info("unsupported ONFI version: %d\n", val);
-		return 0;
+		goto free_onfi_param_page;
+	} else {
+		ret = 1;
 	}
 
 	sanitize_string(p->manufacturer, sizeof(p->manufacturer));
 	sanitize_string(p->model, sizeof(p->model));
+	memcpy(chip->parameters.model, p->model, sizeof(p->model));
 	if (!mtd->name)
-		mtd->name = p->model;
+		mtd->name = chip->parameters.model;
 
 	mtd->writesize = le32_to_cpu(p->byte_per_page);
 
@@ -5270,14 +5278,14 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
 	chip->max_bb_per_die = le16_to_cpu(p->bb_per_lun);
 	chip->blocks_per_die = le32_to_cpu(p->blocks_per_lun);
 
-	if (onfi_feature(chip) & ONFI_FEATURE_16_BIT_BUS)
+	if (le16_to_cpu(p->features) & ONFI_FEATURE_16_BIT_BUS)
 		chip->options |= NAND_BUSWIDTH_16;
 
 	if (p->ecc_bits != 0xff) {
 		chip->ecc_strength_ds = p->ecc_bits;
 		chip->ecc_step_ds = 512;
 	} else if (chip->onfi_version >= 21 &&
-		(onfi_feature(chip) & ONFI_FEATURE_EXT_PARAM_PAGE)) {
+		(le16_to_cpu(p->features) & ONFI_FEATURE_EXT_PARAM_PAGE)) {
 
 		/*
 		 * The nand_flash_detect_ext_param_page() uses the
@@ -5295,7 +5303,20 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
 		pr_warn("Could not retrieve ONFI ECC requirements\n");
 	}
 
-	return 1;
+	/* Save some parameters from the parameter page for future use */
+	if (le16_to_cpu(p->opt_cmd) & ONFI_OPT_CMD_SET_GET_FEATURES)
+		chip->parameters.support_setting_features = true;
+	chip->parameters.t_prog = le16_to_cpu(p->t_prog);
+	chip->parameters.t_bers = le16_to_cpu(p->t_bers);
+	chip->parameters.t_r = le16_to_cpu(p->t_r);
+	chip->parameters.t_ccs = le16_to_cpu(p->t_ccs);
+	chip->parameters.async_timing_mode = le16_to_cpu(p->async_timing_mode);
+	chip->parameters.vendor_revision = le16_to_cpu(p->vendor_revision);
+	memcpy(chip->parameters.vendor, p->vendor, sizeof(p->vendor));
+
+free_onfi_param_page:
+	kfree(p);
+	return ret;
 }
 
 /*
@@ -5304,7 +5325,7 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
 static int nand_flash_detect_jedec(struct nand_chip *chip)
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
-	struct nand_jedec_params *p = &chip->jedec_params;
+	struct nand_jedec_params *p;
 	struct jedec_ecc_info *ecc;
 	char id[5];
 	int i, val, ret;
@@ -5314,14 +5335,23 @@ static int nand_flash_detect_jedec(struct nand_chip *chip)
 	if (ret || strncmp(id, "JEDEC", sizeof(id)))
 		return 0;
 
+	/* JEDEC chip: allocate a buffer to hold its parameter page */
+	p = kzalloc(sizeof(*p), GFP_KERNEL);
+	if (!p)
+		return -ENOMEM;
+
 	ret = nand_read_param_page_op(chip, 0x40, NULL, 0);
-	if (ret)
-		return 0;
+	if (ret) {
+		ret = 0;
+		goto free_jedec_param_page;
+	}
 
 	for (i = 0; i < 3; i++) {
 		ret = nand_read_data_op(chip, p, sizeof(*p), true);
-		if (ret)
-			return 0;
+		if (ret) {
+			ret = 0;
+			goto free_jedec_param_page;
+		}
 
 		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 510) ==
 				le16_to_cpu(p->crc))
@@ -5330,7 +5360,7 @@ static int nand_flash_detect_jedec(struct nand_chip *chip)
 
 	if (i == 3) {
 		pr_err("Could not find valid JEDEC parameter page; aborting\n");
-		return 0;
+		goto free_jedec_param_page;
 	}
 
 	/* Check version */
@@ -5342,13 +5372,14 @@ static int nand_flash_detect_jedec(struct nand_chip *chip)
 
 	if (!chip->jedec_version) {
 		pr_info("unsupported JEDEC version: %d\n", val);
-		return 0;
+		goto free_jedec_param_page;
 	}
 
 	sanitize_string(p->manufacturer, sizeof(p->manufacturer));
 	sanitize_string(p->model, sizeof(p->model));
+	memcpy(chip->parameters.model, p->model, sizeof(p->model));
 	if (!mtd->name)
-		mtd->name = p->model;
+		mtd->name = chip->parameters.model;
 
 	mtd->writesize = le32_to_cpu(p->byte_per_page);
 
@@ -5363,7 +5394,7 @@ static int nand_flash_detect_jedec(struct nand_chip *chip)
 	chip->chipsize *= (uint64_t)mtd->erasesize * p->lun_count;
 	chip->bits_per_cell = p->bits_per_cell;
 
-	if (jedec_feature(chip) & JEDEC_FEATURE_16_BIT_BUS)
+	if (le16_to_cpu(p->features) & JEDEC_FEATURE_16_BIT_BUS)
 		chip->options |= NAND_BUSWIDTH_16;
 
 	/* ECC info */
@@ -5376,7 +5407,9 @@ static int nand_flash_detect_jedec(struct nand_chip *chip)
 		pr_warn("Invalid codeword size\n");
 	}
 
-	return 1;
+free_jedec_param_page:
+	kfree(p);
+	return ret;
 }
 
 /*
@@ -5678,11 +5711,17 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
 	chip->onfi_version = 0;
 	if (!type->name || !type->pagesize) {
 		/* Check if the chip is ONFI compliant */
-		if (nand_flash_detect_onfi(chip))
+		ret = nand_flash_detect_onfi(chip);
+		if (ret < 0)
+			return ret;
+		if (ret)
 			goto ident_done;
 
 		/* Check if the chip is JEDEC compliant */
-		if (nand_flash_detect_jedec(chip))
+		ret = nand_flash_detect_jedec(chip);
+		if (ret < 0)
+			return ret;
+		if (ret)
 			goto ident_done;
 	}
 
@@ -5749,17 +5788,9 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
 
 	pr_info("device found, Manufacturer ID: 0x%02x, Chip ID: 0x%02x\n",
 		maf_id, dev_id);
-
-	if (chip->onfi_version)
-		pr_info("%s %s\n", nand_manufacturer_name(manufacturer),
-			chip->onfi_params.model);
-	else if (chip->jedec_version)
-		pr_info("%s %s\n", nand_manufacturer_name(manufacturer),
-			chip->jedec_params.model);
-	else
-		pr_info("%s %s\n", nand_manufacturer_name(manufacturer),
-			type->name);
-
+	pr_info("%s %s\n", nand_manufacturer_name(manufacturer),
+		(chip->onfi_version || chip->jedec_version) ?
+		chip->parameters.model : type->name);
 	pr_info("%d MiB, %s, erase size: %d KiB, page size: %d, OOB size: %d\n",
 		(int)(chip->chipsize >> 20), nand_is_slc(chip) ? "SLC" : "MLC",
 		mtd->erasesize >> 10, mtd->writesize, mtd->oobsize);
diff --git a/drivers/mtd/nand/nand_micron.c b/drivers/mtd/nand/nand_micron.c
index 48847b441ef7..eaf14885e059 100644
--- a/drivers/mtd/nand/nand_micron.c
+++ b/drivers/mtd/nand/nand_micron.c
@@ -56,17 +56,14 @@ static int micron_nand_setup_read_retry(struct mtd_info *mtd, int retry_mode)
  */
 static int micron_nand_onfi_init(struct nand_chip *chip)
 {
-	struct nand_onfi_params *p = &chip->onfi_params;
+	struct nand_parameters *p = &chip->parameters;
 	struct nand_onfi_vendor_micron *micron = (void *)p->vendor;
 
-	if (!chip->onfi_version)
-		return 0;
-
-	if (le16_to_cpu(p->vendor_revision) < 1)
-		return 0;
+	if (chip->onfi_version && p->vendor_revision) {
+		chip->read_retries = micron->read_retry_options;
+		chip->setup_read_retry = micron_nand_setup_read_retry;
+	}
 
-	chip->read_retries = micron->read_retry_options;
-	chip->setup_read_retry = micron_nand_setup_read_retry;
 
 	return 0;
 }
@@ -237,7 +234,7 @@ static int micron_supports_on_die_ecc(struct nand_chip *chip)
 	 * Some Micron NANDs have an on-die ECC of 4/512, some other
 	 * 8/512. We only support the former.
 	 */
-	if (chip->onfi_params.ecc_bits != 4)
+	if (chip->ecc_strength_ds != 4)
 		return MICRON_ON_DIE_UNSUPPORTED;
 
 	return MICRON_ON_DIE_SUPPORTED;
diff --git a/drivers/mtd/nand/nand_timings.c b/drivers/mtd/nand/nand_timings.c
index 9400d039ddbd..ac140fa4257f 100644
--- a/drivers/mtd/nand/nand_timings.c
+++ b/drivers/mtd/nand/nand_timings.c
@@ -307,16 +307,16 @@ int onfi_fill_data_interface(struct nand_chip *chip,
 	 * These information are part of the ONFI parameter page.
 	 */
 	if (chip->onfi_version) {
-		struct nand_onfi_params *params = &chip->onfi_params;
+		struct nand_parameters *params = &chip->parameters;
 		struct nand_sdr_timings *timings = &iface->timings.sdr;
 
 		/* microseconds -> picoseconds */
-		timings->tPROG_max = 1000000ULL * le16_to_cpu(params->t_prog);
-		timings->tBERS_max = 1000000ULL * le16_to_cpu(params->t_bers);
-		timings->tR_max = 1000000ULL * le16_to_cpu(params->t_r);
+		timings->tPROG_max = 1000000ULL * params->t_prog;
+		timings->tBERS_max = 1000000ULL * params->t_bers;
+		timings->tR_max = 1000000ULL * params->t_r;
 
 		/* nanoseconds -> picoseconds */
-		timings->tCCS_min = 1000UL * le16_to_cpu(params->t_ccs);
+		timings->tCCS_min = 1000UL * params->t_ccs;
 	}
 
 	return 0;
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index 3244f2594b6b..6d8667bb96f4 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -429,6 +429,20 @@ struct nand_jedec_params {
 	__le16 crc;
 } __packed;
 
+struct nand_parameters {
+	/* Generic parameters */
+	char model[20];
+	/* ONFI parameters */
+	bool support_setting_features;
+	u16 t_prog;
+	u16 t_bers;
+	u16 t_r;
+	u16 t_ccs;
+	u16 async_timing_mode;
+	u16 vendor_revision;
+	u8 vendor[88];
+} __packed;
+
 /* The maximum expected count of bytes in the NAND ID sequence */
 #define NAND_MAX_ID_LEN 8
 
@@ -1161,10 +1175,6 @@ int nand_op_parser_exec_op(struct nand_chip *chip,
  *			non 0 if ONFI supported.
  * @jedec_version:	[INTERN] holds the chip JEDEC version (BCD encoded),
  *			non 0 if JEDEC supported.
- * @onfi_params:	[INTERN] holds the ONFI page parameter when ONFI is
- *			supported, 0 otherwise.
- * @jedec_params:	[INTERN] holds the JEDEC parameter page when JEDEC is
- *			supported, 0 otherwise.
  * @max_bb_per_die:	[INTERN] the max number of bad blocks each die of a
  *			this nand device will encounter their life times.
  * @blocks_per_die:	[INTERN] The number of PEBs in a die
@@ -1245,10 +1255,7 @@ struct nand_chip {
 	struct nand_id id;
 	int onfi_version;
 	int jedec_version;
-	union {
-		struct nand_onfi_params	onfi_params;
-		struct nand_jedec_params jedec_params;
-	};
+	struct nand_parameters parameters;
 	u16 max_bb_per_die;
 	u32 blocks_per_die;
 
@@ -1535,26 +1542,13 @@ struct platform_nand_data {
 	struct platform_nand_ctrl ctrl;
 };
 
-/* return the supported features. */
-static inline int onfi_feature(struct nand_chip *chip)
-{
-	return chip->onfi_version ? le16_to_cpu(chip->onfi_params.features) : 0;
-}
-
 /* return the supported asynchronous timing mode. */
 static inline int onfi_get_async_timing_mode(struct nand_chip *chip)
 {
 	if (!chip->onfi_version)
 		return ONFI_TIMING_MODE_UNKNOWN;
-	return le16_to_cpu(chip->onfi_params.async_timing_mode);
-}
 
-/* return the supported synchronous timing mode. */
-static inline int onfi_get_sync_timing_mode(struct nand_chip *chip)
-{
-	if (!chip->onfi_version)
-		return ONFI_TIMING_MODE_UNKNOWN;
-	return le16_to_cpu(chip->onfi_params.src_sync_timing_mode);
+	return chip->parameters.async_timing_mode;
 }
 
 int onfi_fill_data_interface(struct nand_chip *chip,
@@ -1591,13 +1585,6 @@ static inline int nand_opcode_8bits(unsigned int command)
 	return 0;
 }
 
-/* return the supported JEDEC features. */
-static inline int jedec_feature(struct nand_chip *chip)
-{
-	return chip->jedec_version ? le16_to_cpu(chip->jedec_params.features)
-		: 0;
-}
-
 /* get timing characteristics from ONFI timing mode. */
 const struct nand_sdr_timings *onfi_async_timing_mode_to_sdr_timings(int mode);
 
-- 
2.14.1

Re: [PATCH 4/6] mtd: nand: Stop using a static parameter page for all chips

[Boris Brezillon]

On Sat,  3 Feb 2018 10:55:42 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> From: Miquel Raynal <miquel.raynal@free-electrons.com>
> 
> The NAND chip parameter page is statically allocated within the
> nand_chip structure, which reserves a lot of space. Even not ONFI nor
> JEDEC chips have it embedded. Also, only a few parameters are still be
> read from the parameter page after the detection.
> 
> Remove these pages from the nand_chip structure and instead create a
> small structure with all the parameters that will be needed outside of
> the identification phase.

Can we move this change at the end of the patch series? I'm not
entirely sure where/how I want to store the information extracted from
the ONFI param page, and I don't want to block the other changes just
for that.

> 
> During identification, allocate dynamically the parameter page after the
> chip is declared ONFI or JEDEC. Extract all information from it and free
> this space when exiting.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> ---
>  drivers/mtd/nand/nand_base.c    | 111 +++++++++++++++++++++++++---------------
>  drivers/mtd/nand/nand_micron.c  |  15 +++---
>  drivers/mtd/nand/nand_timings.c |  10 ++--
>  include/linux/mtd/rawnand.h     |  45 ++++++----------
>  4 files changed, 98 insertions(+), 83 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 8d2c37011979..4a879b1635b3 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -1174,9 +1174,7 @@ int nand_get_features(struct nand_chip *chip, int addr,
>  {
>  	struct mtd_info *mtd = nand_to_mtd(chip);
>  
> -	if (!chip->onfi_version ||
> -	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
> -	      & ONFI_OPT_CMD_SET_GET_FEATURES))
> +	if (!chip->parameters.support_setting_features)
>  		return -ENOTSUPP;
>  
>  	return chip->onfi_get_features(mtd, chip, addr, subfeature_param);
> @@ -1197,9 +1195,7 @@ int nand_set_features(struct nand_chip *chip, int addr,
>  {
>  	struct mtd_info *mtd = nand_to_mtd(chip);
>  
> -	if (!chip->onfi_version ||
> -	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
> -	      & ONFI_OPT_CMD_SET_GET_FEATURES))
> +	if (!chip->parameters.support_setting_features)
>  		return -ENOTSUPP;
>  
>  	return chip->onfi_set_features(mtd, chip, addr, subfeature_param);
> @@ -5198,7 +5194,7 @@ static int nand_flash_detect_ext_param_page(struct nand_chip *chip,
>  static int nand_flash_detect_onfi(struct nand_chip *chip)
>  {
>  	struct mtd_info *mtd = nand_to_mtd(chip);
> -	struct nand_onfi_params *p = &chip->onfi_params;
> +	struct nand_onfi_params *p;
>  	char id[4];
>  	int i, ret, val;
>  
> @@ -5207,14 +5203,23 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
>  	if (ret || strncmp(id, "ONFI", 4))
>  		return 0;
>  
> +	/* ONFI chip: allocate a buffer to hold its parameter page */
> +	p = kzalloc(sizeof(*p), GFP_KERNEL);
> +	if (!p)
> +		return -ENOMEM;
> +
>  	ret = nand_read_param_page_op(chip, 0, NULL, 0);
> -	if (ret)
> -		return 0;
> +	if (ret) {
> +		ret = 0;
> +		goto free_onfi_param_page;
> +	}
>  
>  	for (i = 0; i < 3; i++) {
>  		ret = nand_read_data_op(chip, p, sizeof(*p), true);
> -		if (ret)
> -			return 0;
> +		if (ret) {
> +			ret = 0;
> +			goto free_onfi_param_page;
> +		}
>  
>  		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
>  				le16_to_cpu(p->crc)) {
> @@ -5224,7 +5229,7 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
>  
>  	if (i == 3) {
>  		pr_err("Could not find valid ONFI parameter page; aborting\n");
> -		return 0;
> +		goto free_onfi_param_page;
>  	}
>  
>  	/* Check version */
> @@ -5242,13 +5247,16 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
>  
>  	if (!chip->onfi_version) {
>  		pr_info("unsupported ONFI version: %d\n", val);
> -		return 0;
> +		goto free_onfi_param_page;
> +	} else {
> +		ret = 1;
>  	}
>  
>  	sanitize_string(p->manufacturer, sizeof(p->manufacturer));
>  	sanitize_string(p->model, sizeof(p->model));
> +	memcpy(chip->parameters.model, p->model, sizeof(p->model));
>  	if (!mtd->name)
> -		mtd->name = p->model;
> +		mtd->name = chip->parameters.model;
>  
>  	mtd->writesize = le32_to_cpu(p->byte_per_page);
>  
> @@ -5270,14 +5278,14 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
>  	chip->max_bb_per_die = le16_to_cpu(p->bb_per_lun);
>  	chip->blocks_per_die = le32_to_cpu(p->blocks_per_lun);
>  
> -	if (onfi_feature(chip) & ONFI_FEATURE_16_BIT_BUS)
> +	if (le16_to_cpu(p->features) & ONFI_FEATURE_16_BIT_BUS)
>  		chip->options |= NAND_BUSWIDTH_16;
>  
>  	if (p->ecc_bits != 0xff) {
>  		chip->ecc_strength_ds = p->ecc_bits;
>  		chip->ecc_step_ds = 512;
>  	} else if (chip->onfi_version >= 21 &&
> -		(onfi_feature(chip) & ONFI_FEATURE_EXT_PARAM_PAGE)) {
> +		(le16_to_cpu(p->features) & ONFI_FEATURE_EXT_PARAM_PAGE)) {
>  
>  		/*
>  		 * The nand_flash_detect_ext_param_page() uses the
> @@ -5295,7 +5303,20 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
>  		pr_warn("Could not retrieve ONFI ECC requirements\n");
>  	}
>  
> -	return 1;
> +	/* Save some parameters from the parameter page for future use */
> +	if (le16_to_cpu(p->opt_cmd) & ONFI_OPT_CMD_SET_GET_FEATURES)
> +		chip->parameters.support_setting_features = true;
> +	chip->parameters.t_prog = le16_to_cpu(p->t_prog);
> +	chip->parameters.t_bers = le16_to_cpu(p->t_bers);
> +	chip->parameters.t_r = le16_to_cpu(p->t_r);
> +	chip->parameters.t_ccs = le16_to_cpu(p->t_ccs);
> +	chip->parameters.async_timing_mode = le16_to_cpu(p->async_timing_mode);
> +	chip->parameters.vendor_revision = le16_to_cpu(p->vendor_revision);
> +	memcpy(chip->parameters.vendor, p->vendor, sizeof(p->vendor));
> +
> +free_onfi_param_page:
> +	kfree(p);
> +	return ret;
>  }
>  
>  /*
> @@ -5304,7 +5325,7 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
>  static int nand_flash_detect_jedec(struct nand_chip *chip)
>  {
>  	struct mtd_info *mtd = nand_to_mtd(chip);
> -	struct nand_jedec_params *p = &chip->jedec_params;
> +	struct nand_jedec_params *p;
>  	struct jedec_ecc_info *ecc;
>  	char id[5];
>  	int i, val, ret;
> @@ -5314,14 +5335,23 @@ static int nand_flash_detect_jedec(struct nand_chip *chip)
>  	if (ret || strncmp(id, "JEDEC", sizeof(id)))
>  		return 0;
>  
> +	/* JEDEC chip: allocate a buffer to hold its parameter page */
> +	p = kzalloc(sizeof(*p), GFP_KERNEL);
> +	if (!p)
> +		return -ENOMEM;
> +
>  	ret = nand_read_param_page_op(chip, 0x40, NULL, 0);
> -	if (ret)
> -		return 0;
> +	if (ret) {
> +		ret = 0;
> +		goto free_jedec_param_page;
> +	}
>  
>  	for (i = 0; i < 3; i++) {
>  		ret = nand_read_data_op(chip, p, sizeof(*p), true);
> -		if (ret)
> -			return 0;
> +		if (ret) {
> +			ret = 0;
> +			goto free_jedec_param_page;
> +		}
>  
>  		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 510) ==
>  				le16_to_cpu(p->crc))
> @@ -5330,7 +5360,7 @@ static int nand_flash_detect_jedec(struct nand_chip *chip)
>  
>  	if (i == 3) {
>  		pr_err("Could not find valid JEDEC parameter page; aborting\n");
> -		return 0;
> +		goto free_jedec_param_page;
>  	}
>  
>  	/* Check version */
> @@ -5342,13 +5372,14 @@ static int nand_flash_detect_jedec(struct nand_chip *chip)
>  
>  	if (!chip->jedec_version) {
>  		pr_info("unsupported JEDEC version: %d\n", val);
> -		return 0;
> +		goto free_jedec_param_page;
>  	}
>  
>  	sanitize_string(p->manufacturer, sizeof(p->manufacturer));
>  	sanitize_string(p->model, sizeof(p->model));
> +	memcpy(chip->parameters.model, p->model, sizeof(p->model));
>  	if (!mtd->name)
> -		mtd->name = p->model;
> +		mtd->name = chip->parameters.model;
>  
>  	mtd->writesize = le32_to_cpu(p->byte_per_page);
>  
> @@ -5363,7 +5394,7 @@ static int nand_flash_detect_jedec(struct nand_chip *chip)
>  	chip->chipsize *= (uint64_t)mtd->erasesize * p->lun_count;
>  	chip->bits_per_cell = p->bits_per_cell;
>  
> -	if (jedec_feature(chip) & JEDEC_FEATURE_16_BIT_BUS)
> +	if (le16_to_cpu(p->features) & JEDEC_FEATURE_16_BIT_BUS)
>  		chip->options |= NAND_BUSWIDTH_16;
>  
>  	/* ECC info */
> @@ -5376,7 +5407,9 @@ static int nand_flash_detect_jedec(struct nand_chip *chip)
>  		pr_warn("Invalid codeword size\n");
>  	}
>  
> -	return 1;
> +free_jedec_param_page:
> +	kfree(p);
> +	return ret;
>  }

Looks like the JEDEC param page is not needed after nand_scan_ident()
has done its job, so this one is easy to get rid of. Could you do this
change (get rid of chip->jedec_param) in a separate patch so that I can
apply it independently.

>  
>  /*
> @@ -5678,11 +5711,17 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
>  	chip->onfi_version = 0;
>  	if (!type->name || !type->pagesize) {
>  		/* Check if the chip is ONFI compliant */
> -		if (nand_flash_detect_onfi(chip))
> +		ret = nand_flash_detect_onfi(chip);
> +		if (ret < 0)
> +			return ret;
> +		if (ret)
>  			goto ident_done;
>  
>  		/* Check if the chip is JEDEC compliant */
> -		if (nand_flash_detect_jedec(chip))
> +		ret = nand_flash_detect_jedec(chip);
> +		if (ret < 0)
> +			return ret;
> +		if (ret)
>  			goto ident_done;
>  	}
>  
> @@ -5749,17 +5788,9 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
>  
>  	pr_info("device found, Manufacturer ID: 0x%02x, Chip ID: 0x%02x\n",
>  		maf_id, dev_id);
> -
> -	if (chip->onfi_version)
> -		pr_info("%s %s\n", nand_manufacturer_name(manufacturer),
> -			chip->onfi_params.model);
> -	else if (chip->jedec_version)
> -		pr_info("%s %s\n", nand_manufacturer_name(manufacturer),
> -			chip->jedec_params.model);
> -	else
> -		pr_info("%s %s\n", nand_manufacturer_name(manufacturer),
> -			type->name);
> -
> +	pr_info("%s %s\n", nand_manufacturer_name(manufacturer),
> +		(chip->onfi_version || chip->jedec_version) ?
> +		chip->parameters.model : type->name);
>  	pr_info("%d MiB, %s, erase size: %d KiB, page size: %d, OOB size: %d\n",
>  		(int)(chip->chipsize >> 20), nand_is_slc(chip) ? "SLC" : "MLC",
>  		mtd->erasesize >> 10, mtd->writesize, mtd->oobsize);
> diff --git a/drivers/mtd/nand/nand_micron.c b/drivers/mtd/nand/nand_micron.c
> index 48847b441ef7..eaf14885e059 100644
> --- a/drivers/mtd/nand/nand_micron.c
> +++ b/drivers/mtd/nand/nand_micron.c
> @@ -56,17 +56,14 @@ static int micron_nand_setup_read_retry(struct mtd_info *mtd, int retry_mode)
>   */
>  static int micron_nand_onfi_init(struct nand_chip *chip)
>  {
> -	struct nand_onfi_params *p = &chip->onfi_params;
> +	struct nand_parameters *p = &chip->parameters;
>  	struct nand_onfi_vendor_micron *micron = (void *)p->vendor;
>  
> -	if (!chip->onfi_version)
> -		return 0;
> -
> -	if (le16_to_cpu(p->vendor_revision) < 1)
> -		return 0;
> +	if (chip->onfi_version && p->vendor_revision) {
> +		chip->read_retries = micron->read_retry_options;
> +		chip->setup_read_retry = micron_nand_setup_read_retry;
> +	}
>  
> -	chip->read_retries = micron->read_retry_options;
> -	chip->setup_read_retry = micron_nand_setup_read_retry;
>  
>  	return 0;
>  }
> @@ -237,7 +234,7 @@ static int micron_supports_on_die_ecc(struct nand_chip *chip)
>  	 * Some Micron NANDs have an on-die ECC of 4/512, some other
>  	 * 8/512. We only support the former.
>  	 */
> -	if (chip->onfi_params.ecc_bits != 4)
> +	if (chip->ecc_strength_ds != 4)
>  		return MICRON_ON_DIE_UNSUPPORTED;
>  
>  	return MICRON_ON_DIE_SUPPORTED;
> diff --git a/drivers/mtd/nand/nand_timings.c b/drivers/mtd/nand/nand_timings.c
> index 9400d039ddbd..ac140fa4257f 100644
> --- a/drivers/mtd/nand/nand_timings.c
> +++ b/drivers/mtd/nand/nand_timings.c
> @@ -307,16 +307,16 @@ int onfi_fill_data_interface(struct nand_chip *chip,
>  	 * These information are part of the ONFI parameter page.
>  	 */
>  	if (chip->onfi_version) {
> -		struct nand_onfi_params *params = &chip->onfi_params;
> +		struct nand_parameters *params = &chip->parameters;
>  		struct nand_sdr_timings *timings = &iface->timings.sdr;
>  
>  		/* microseconds -> picoseconds */
> -		timings->tPROG_max = 1000000ULL * le16_to_cpu(params->t_prog);
> -		timings->tBERS_max = 1000000ULL * le16_to_cpu(params->t_bers);
> -		timings->tR_max = 1000000ULL * le16_to_cpu(params->t_r);
> +		timings->tPROG_max = 1000000ULL * params->t_prog;
> +		timings->tBERS_max = 1000000ULL * params->t_bers;
> +		timings->tR_max = 1000000ULL * params->t_r;
>  
>  		/* nanoseconds -> picoseconds */
> -		timings->tCCS_min = 1000UL * le16_to_cpu(params->t_ccs);
> +		timings->tCCS_min = 1000UL * params->t_ccs;
>  	}
>  
>  	return 0;
> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index 3244f2594b6b..6d8667bb96f4 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -429,6 +429,20 @@ struct nand_jedec_params {
>  	__le16 crc;
>  } __packed;
>  
> +struct nand_parameters {
> +	/* Generic parameters */
> +	char model[20];

Why not const char *? I know the model name is stored in a 20 bytes
array in the ONFI and JEDEC param page, but I'm not sure 20 bytes will
be enough for non-JEDEC/ONFI NANDs.

> +	/* ONFI parameters */
> +	bool support_setting_features;
> +	u16 t_prog;
> +	u16 t_bers;
> +	u16 t_r;
> +	u16 t_ccs;
> +	u16 async_timing_mode;
> +	u16 vendor_revision;
> +	u8 vendor[88];

That's clearly the part I don't like. If we have ONFI specific
information that we want/need to keep around, they should be placed in a
different struct (with onfi in its name), and it should probably be
dynamically allocated to not grow the nand_chip struct. I know
allocating things in nand_scan_ident() is forbidden and that's why you
stored things directly in nand_chip, but maybe we should address that
problem instead of continuously find alternative solutions every time
we need to allocate something from nand_scan_ident()...

> +} __packed;

I'm pretty sure __packed is not needed here.

> +
>  /* The maximum expected count of bytes in the NAND ID sequence */
>  #define NAND_MAX_ID_LEN 8
>  
> @@ -1161,10 +1175,6 @@ int nand_op_parser_exec_op(struct nand_chip *chip,
>   *			non 0 if ONFI supported.
>   * @jedec_version:	[INTERN] holds the chip JEDEC version (BCD encoded),
>   *			non 0 if JEDEC supported.
> - * @onfi_params:	[INTERN] holds the ONFI page parameter when ONFI is
> - *			supported, 0 otherwise.
> - * @jedec_params:	[INTERN] holds the JEDEC parameter page when JEDEC is
> - *			supported, 0 otherwise.
>   * @max_bb_per_die:	[INTERN] the max number of bad blocks each die of a
>   *			this nand device will encounter their life times.
>   * @blocks_per_die:	[INTERN] The number of PEBs in a die
> @@ -1245,10 +1255,7 @@ struct nand_chip {
>  	struct nand_id id;
>  	int onfi_version;
>  	int jedec_version;
> -	union {
> -		struct nand_onfi_params	onfi_params;
> -		struct nand_jedec_params jedec_params;
> -	};
> +	struct nand_parameters parameters;
>  	u16 max_bb_per_die;
>  	u32 blocks_per_die;
>  
> @@ -1535,26 +1542,13 @@ struct platform_nand_data {
>  	struct platform_nand_ctrl ctrl;
>  };
>  
> -/* return the supported features. */
> -static inline int onfi_feature(struct nand_chip *chip)
> -{
> -	return chip->onfi_version ? le16_to_cpu(chip->onfi_params.features) : 0;
> -}
> -
>  /* return the supported asynchronous timing mode. */
>  static inline int onfi_get_async_timing_mode(struct nand_chip *chip)
>  {
>  	if (!chip->onfi_version)
>  		return ONFI_TIMING_MODE_UNKNOWN;
> -	return le16_to_cpu(chip->onfi_params.async_timing_mode);
> -}
>  
> -/* return the supported synchronous timing mode. */
> -static inline int onfi_get_sync_timing_mode(struct nand_chip *chip)
> -{
> -	if (!chip->onfi_version)
> -		return ONFI_TIMING_MODE_UNKNOWN;
> -	return le16_to_cpu(chip->onfi_params.src_sync_timing_mode);
> +	return chip->parameters.async_timing_mode;
>  }
>  
>  int onfi_fill_data_interface(struct nand_chip *chip,
> @@ -1591,13 +1585,6 @@ static inline int nand_opcode_8bits(unsigned int command)
>  	return 0;
>  }
>  
> -/* return the supported JEDEC features. */
> -static inline int jedec_feature(struct nand_chip *chip)
> -{
> -	return chip->jedec_version ? le16_to_cpu(chip->jedec_params.features)
> -		: 0;
> -}
> -
>  /* get timing characteristics from ONFI timing mode. */
>  const struct nand_sdr_timings *onfi_async_timing_mode_to_sdr_timings(int mode);
>  



-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH 4/6] mtd: nand: Stop using a static parameter page for all chips

[Miquel Raynal]

Hi Boris,

On Wed, 14 Feb 2018 09:41:45 +0100, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:

> On Sat,  3 Feb 2018 10:55:42 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > From: Miquel Raynal <miquel.raynal@free-electrons.com>
> > 
> > The NAND chip parameter page is statically allocated within the
> > nand_chip structure, which reserves a lot of space. Even not ONFI nor
> > JEDEC chips have it embedded. Also, only a few parameters are still be
> > read from the parameter page after the detection.
> > 
> > Remove these pages from the nand_chip structure and instead create a
> > small structure with all the parameters that will be needed outside of
> > the identification phase.  
> 
> Can we move this change at the end of the patch series? I'm not
> entirely sure where/how I want to store the information extracted from
> the ONFI param page, and I don't want to block the other changes just
> for that.

As discussed elsewhere, I propose to keep a static "nand_parameters"
structure in the nand_chip structure with an ONFI parameters structure
inside. Aside, I am working on removing the "no allocation in
nand_scan_ident()" limitation, this way I will then remove the
ONFI-related structure and allocate it dynamically only when needed as
soon as this second series will be ready.

However, I did split some patches and reorganized them in order for you
to get rid of the parameter pages only when you'll be ready for it
without blocking the rest of the series. See the next iteration.

> 
> > 
> > During identification, allocate dynamically the parameter page after the
> > chip is declared ONFI or JEDEC. Extract all information from it and free
> > this space when exiting.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> > ---
> >  drivers/mtd/nand/nand_base.c    | 111 +++++++++++++++++++++++++---------------
> >  drivers/mtd/nand/nand_micron.c  |  15 +++---
> >  drivers/mtd/nand/nand_timings.c |  10 ++--
> >  include/linux/mtd/rawnand.h     |  45 ++++++----------
> >  4 files changed, 98 insertions(+), 83 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index 8d2c37011979..4a879b1635b3 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -1174,9 +1174,7 @@ int nand_get_features(struct nand_chip *chip, int addr,
> >  {
> >  	struct mtd_info *mtd = nand_to_mtd(chip);
> >  
> > -	if (!chip->onfi_version ||
> > -	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
> > -	      & ONFI_OPT_CMD_SET_GET_FEATURES))
> > +	if (!chip->parameters.support_setting_features)
> >  		return -ENOTSUPP;
> >  
> >  	return chip->onfi_get_features(mtd, chip, addr, subfeature_param);
> > @@ -1197,9 +1195,7 @@ int nand_set_features(struct nand_chip *chip, int addr,
> >  {
> >  	struct mtd_info *mtd = nand_to_mtd(chip);
> >  
> > -	if (!chip->onfi_version ||
> > -	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
> > -	      & ONFI_OPT_CMD_SET_GET_FEATURES))
> > +	if (!chip->parameters.support_setting_features)
> >  		return -ENOTSUPP;
> >  
> >  	return chip->onfi_set_features(mtd, chip, addr, subfeature_param);
> > @@ -5198,7 +5194,7 @@ static int nand_flash_detect_ext_param_page(struct nand_chip *chip,
> >  static int nand_flash_detect_onfi(struct nand_chip *chip)
> >  {
> >  	struct mtd_info *mtd = nand_to_mtd(chip);
> > -	struct nand_onfi_params *p = &chip->onfi_params;
> > +	struct nand_onfi_params *p;
> >  	char id[4];
> >  	int i, ret, val;
> >  
> > @@ -5207,14 +5203,23 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
> >  	if (ret || strncmp(id, "ONFI", 4))
> >  		return 0;
> >  
> > +	/* ONFI chip: allocate a buffer to hold its parameter page */
> > +	p = kzalloc(sizeof(*p), GFP_KERNEL);
> > +	if (!p)
> > +		return -ENOMEM;
> > +
> >  	ret = nand_read_param_page_op(chip, 0, NULL, 0);
> > -	if (ret)
> > -		return 0;
> > +	if (ret) {
> > +		ret = 0;
> > +		goto free_onfi_param_page;
> > +	}
> >  
> >  	for (i = 0; i < 3; i++) {
> >  		ret = nand_read_data_op(chip, p, sizeof(*p), true);
> > -		if (ret)
> > -			return 0;
> > +		if (ret) {
> > +			ret = 0;
> > +			goto free_onfi_param_page;
> > +		}
> >  
> >  		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
> >  				le16_to_cpu(p->crc)) {
> > @@ -5224,7 +5229,7 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
> >  
> >  	if (i == 3) {
> >  		pr_err("Could not find valid ONFI parameter page; aborting\n");
> > -		return 0;
> > +		goto free_onfi_param_page;
> >  	}
> >  
> >  	/* Check version */
> > @@ -5242,13 +5247,16 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
> >  
> >  	if (!chip->onfi_version) {
> >  		pr_info("unsupported ONFI version: %d\n", val);
> > -		return 0;
> > +		goto free_onfi_param_page;
> > +	} else {
> > +		ret = 1;
> >  	}
> >  
> >  	sanitize_string(p->manufacturer, sizeof(p->manufacturer));
> >  	sanitize_string(p->model, sizeof(p->model));
> > +	memcpy(chip->parameters.model, p->model, sizeof(p->model));
> >  	if (!mtd->name)
> > -		mtd->name = p->model;
> > +		mtd->name = chip->parameters.model;
> >  
> >  	mtd->writesize = le32_to_cpu(p->byte_per_page);
> >  
> > @@ -5270,14 +5278,14 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
> >  	chip->max_bb_per_die = le16_to_cpu(p->bb_per_lun);
> >  	chip->blocks_per_die = le32_to_cpu(p->blocks_per_lun);
> >  
> > -	if (onfi_feature(chip) & ONFI_FEATURE_16_BIT_BUS)
> > +	if (le16_to_cpu(p->features) & ONFI_FEATURE_16_BIT_BUS)
> >  		chip->options |= NAND_BUSWIDTH_16;
> >  
> >  	if (p->ecc_bits != 0xff) {
> >  		chip->ecc_strength_ds = p->ecc_bits;
> >  		chip->ecc_step_ds = 512;
> >  	} else if (chip->onfi_version >= 21 &&
> > -		(onfi_feature(chip) & ONFI_FEATURE_EXT_PARAM_PAGE)) {
> > +		(le16_to_cpu(p->features) & ONFI_FEATURE_EXT_PARAM_PAGE)) {
> >  
> >  		/*
> >  		 * The nand_flash_detect_ext_param_page() uses the
> > @@ -5295,7 +5303,20 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
> >  		pr_warn("Could not retrieve ONFI ECC requirements\n");
> >  	}
> >  
> > -	return 1;
> > +	/* Save some parameters from the parameter page for future use */
> > +	if (le16_to_cpu(p->opt_cmd) & ONFI_OPT_CMD_SET_GET_FEATURES)
> > +		chip->parameters.support_setting_features = true;
> > +	chip->parameters.t_prog = le16_to_cpu(p->t_prog);
> > +	chip->parameters.t_bers = le16_to_cpu(p->t_bers);
> > +	chip->parameters.t_r = le16_to_cpu(p->t_r);
> > +	chip->parameters.t_ccs = le16_to_cpu(p->t_ccs);
> > +	chip->parameters.async_timing_mode = le16_to_cpu(p->async_timing_mode);
> > +	chip->parameters.vendor_revision = le16_to_cpu(p->vendor_revision);
> > +	memcpy(chip->parameters.vendor, p->vendor, sizeof(p->vendor));
> > +
> > +free_onfi_param_page:
> > +	kfree(p);
> > +	return ret;
> >  }
> >  
> >  /*
> > @@ -5304,7 +5325,7 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
> >  static int nand_flash_detect_jedec(struct nand_chip *chip)
> >  {
> >  	struct mtd_info *mtd = nand_to_mtd(chip);
> > -	struct nand_jedec_params *p = &chip->jedec_params;
> > +	struct nand_jedec_params *p;
> >  	struct jedec_ecc_info *ecc;
> >  	char id[5];
> >  	int i, val, ret;
> > @@ -5314,14 +5335,23 @@ static int nand_flash_detect_jedec(struct nand_chip *chip)
> >  	if (ret || strncmp(id, "JEDEC", sizeof(id)))
> >  		return 0;
> >  
> > +	/* JEDEC chip: allocate a buffer to hold its parameter page */
> > +	p = kzalloc(sizeof(*p), GFP_KERNEL);
> > +	if (!p)
> > +		return -ENOMEM;
> > +
> >  	ret = nand_read_param_page_op(chip, 0x40, NULL, 0);
> > -	if (ret)
> > -		return 0;
> > +	if (ret) {
> > +		ret = 0;
> > +		goto free_jedec_param_page;
> > +	}
> >  
> >  	for (i = 0; i < 3; i++) {
> >  		ret = nand_read_data_op(chip, p, sizeof(*p), true);
> > -		if (ret)
> > -			return 0;
> > +		if (ret) {
> > +			ret = 0;
> > +			goto free_jedec_param_page;
> > +		}
> >  
> >  		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 510) ==
> >  				le16_to_cpu(p->crc))
> > @@ -5330,7 +5360,7 @@ static int nand_flash_detect_jedec(struct nand_chip *chip)
> >  
> >  	if (i == 3) {
> >  		pr_err("Could not find valid JEDEC parameter page; aborting\n");
> > -		return 0;
> > +		goto free_jedec_param_page;
> >  	}
> >  
> >  	/* Check version */
> > @@ -5342,13 +5372,14 @@ static int nand_flash_detect_jedec(struct nand_chip *chip)
> >  
> >  	if (!chip->jedec_version) {
> >  		pr_info("unsupported JEDEC version: %d\n", val);
> > -		return 0;
> > +		goto free_jedec_param_page;
> >  	}
> >  
> >  	sanitize_string(p->manufacturer, sizeof(p->manufacturer));
> >  	sanitize_string(p->model, sizeof(p->model));
> > +	memcpy(chip->parameters.model, p->model, sizeof(p->model));
> >  	if (!mtd->name)
> > -		mtd->name = p->model;
> > +		mtd->name = chip->parameters.model;
> >  
> >  	mtd->writesize = le32_to_cpu(p->byte_per_page);
> >  
> > @@ -5363,7 +5394,7 @@ static int nand_flash_detect_jedec(struct nand_chip *chip)
> >  	chip->chipsize *= (uint64_t)mtd->erasesize * p->lun_count;
> >  	chip->bits_per_cell = p->bits_per_cell;
> >  
> > -	if (jedec_feature(chip) & JEDEC_FEATURE_16_BIT_BUS)
> > +	if (le16_to_cpu(p->features) & JEDEC_FEATURE_16_BIT_BUS)
> >  		chip->options |= NAND_BUSWIDTH_16;
> >  
> >  	/* ECC info */
> > @@ -5376,7 +5407,9 @@ static int nand_flash_detect_jedec(struct nand_chip *chip)
> >  		pr_warn("Invalid codeword size\n");
> >  	}
> >  
> > -	return 1;
> > +free_jedec_param_page:
> > +	kfree(p);
> > +	return ret;
> >  }  
> 
> Looks like the JEDEC param page is not needed after nand_scan_ident()
> has done its job, so this one is easy to get rid of. Could you do this
> change (get rid of chip->jedec_param) in a separate patch so that I can
> apply it independently.

Okay

> 
> >  
> >  /*
> > @@ -5678,11 +5711,17 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
> >  	chip->onfi_version = 0;
> >  	if (!type->name || !type->pagesize) {
> >  		/* Check if the chip is ONFI compliant */
> > -		if (nand_flash_detect_onfi(chip))
> > +		ret = nand_flash_detect_onfi(chip);
> > +		if (ret < 0)
> > +			return ret;
> > +		if (ret)
> >  			goto ident_done;
> >  
> >  		/* Check if the chip is JEDEC compliant */
> > -		if (nand_flash_detect_jedec(chip))
> > +		ret = nand_flash_detect_jedec(chip);
> > +		if (ret < 0)
> > +			return ret;
> > +		if (ret)
> >  			goto ident_done;
> >  	}
> >  
> > @@ -5749,17 +5788,9 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
> >  
> >  	pr_info("device found, Manufacturer ID: 0x%02x, Chip ID: 0x%02x\n",
> >  		maf_id, dev_id);
> > -
> > -	if (chip->onfi_version)
> > -		pr_info("%s %s\n", nand_manufacturer_name(manufacturer),
> > -			chip->onfi_params.model);
> > -	else if (chip->jedec_version)
> > -		pr_info("%s %s\n", nand_manufacturer_name(manufacturer),
> > -			chip->jedec_params.model);
> > -	else
> > -		pr_info("%s %s\n", nand_manufacturer_name(manufacturer),
> > -			type->name);
> > -
> > +	pr_info("%s %s\n", nand_manufacturer_name(manufacturer),
> > +		(chip->onfi_version || chip->jedec_version) ?
> > +		chip->parameters.model : type->name);
> >  	pr_info("%d MiB, %s, erase size: %d KiB, page size: %d, OOB size: %d\n",
> >  		(int)(chip->chipsize >> 20), nand_is_slc(chip) ? "SLC" : "MLC",
> >  		mtd->erasesize >> 10, mtd->writesize, mtd->oobsize);
> > diff --git a/drivers/mtd/nand/nand_micron.c b/drivers/mtd/nand/nand_micron.c
> > index 48847b441ef7..eaf14885e059 100644
> > --- a/drivers/mtd/nand/nand_micron.c
> > +++ b/drivers/mtd/nand/nand_micron.c
> > @@ -56,17 +56,14 @@ static int micron_nand_setup_read_retry(struct mtd_info *mtd, int retry_mode)
> >   */
> >  static int micron_nand_onfi_init(struct nand_chip *chip)
> >  {
> > -	struct nand_onfi_params *p = &chip->onfi_params;
> > +	struct nand_parameters *p = &chip->parameters;
> >  	struct nand_onfi_vendor_micron *micron = (void *)p->vendor;
> >  
> > -	if (!chip->onfi_version)
> > -		return 0;
> > -
> > -	if (le16_to_cpu(p->vendor_revision) < 1)
> > -		return 0;
> > +	if (chip->onfi_version && p->vendor_revision) {
> > +		chip->read_retries = micron->read_retry_options;
> > +		chip->setup_read_retry = micron_nand_setup_read_retry;
> > +	}
> >  
> > -	chip->read_retries = micron->read_retry_options;
> > -	chip->setup_read_retry = micron_nand_setup_read_retry;
> >  
> >  	return 0;
> >  }
> > @@ -237,7 +234,7 @@ static int micron_supports_on_die_ecc(struct nand_chip *chip)
> >  	 * Some Micron NANDs have an on-die ECC of 4/512, some other
> >  	 * 8/512. We only support the former.
> >  	 */
> > -	if (chip->onfi_params.ecc_bits != 4)
> > +	if (chip->ecc_strength_ds != 4)
> >  		return MICRON_ON_DIE_UNSUPPORTED;
> >  
> >  	return MICRON_ON_DIE_SUPPORTED;
> > diff --git a/drivers/mtd/nand/nand_timings.c b/drivers/mtd/nand/nand_timings.c
> > index 9400d039ddbd..ac140fa4257f 100644
> > --- a/drivers/mtd/nand/nand_timings.c
> > +++ b/drivers/mtd/nand/nand_timings.c
> > @@ -307,16 +307,16 @@ int onfi_fill_data_interface(struct nand_chip *chip,
> >  	 * These information are part of the ONFI parameter page.
> >  	 */
> >  	if (chip->onfi_version) {
> > -		struct nand_onfi_params *params = &chip->onfi_params;
> > +		struct nand_parameters *params = &chip->parameters;
> >  		struct nand_sdr_timings *timings = &iface->timings.sdr;
> >  
> >  		/* microseconds -> picoseconds */
> > -		timings->tPROG_max = 1000000ULL * le16_to_cpu(params->t_prog);
> > -		timings->tBERS_max = 1000000ULL * le16_to_cpu(params->t_bers);
> > -		timings->tR_max = 1000000ULL * le16_to_cpu(params->t_r);
> > +		timings->tPROG_max = 1000000ULL * params->t_prog;
> > +		timings->tBERS_max = 1000000ULL * params->t_bers;
> > +		timings->tR_max = 1000000ULL * params->t_r;
> >  
> >  		/* nanoseconds -> picoseconds */
> > -		timings->tCCS_min = 1000UL * le16_to_cpu(params->t_ccs);
> > +		timings->tCCS_min = 1000UL * params->t_ccs;
> >  	}
> >  
> >  	return 0;
> > diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> > index 3244f2594b6b..6d8667bb96f4 100644
> > --- a/include/linux/mtd/rawnand.h
> > +++ b/include/linux/mtd/rawnand.h
> > @@ -429,6 +429,20 @@ struct nand_jedec_params {
> >  	__le16 crc;
> >  } __packed;
> >  
> > +struct nand_parameters {
> > +	/* Generic parameters */
> > +	char model[20];  
> 
> Why not const char *? I know the model name is stored in a 20 bytes

For now I will keep a char model[]. As soon as the limitation about
dynamic allocations at identification time is removed, I will switch
this field to const char *.

> array in the ONFI and JEDEC param page, but I'm not sure 20 bytes will
> be enough for non-JEDEC/ONFI NANDs.

I will set it to 100 then, that should be enough. In the near future it
will be possible to allocate dynamically the right size anyway.

> 
> > +	/* ONFI parameters */
> > +	bool support_setting_features;
> > +	u16 t_prog;
> > +	u16 t_bers;
> > +	u16 t_r;
> > +	u16 t_ccs;
> > +	u16 async_timing_mode;
> > +	u16 vendor_revision;
> > +	u8 vendor[88];  
> 
> That's clearly the part I don't like. If we have ONFI specific
> information that we want/need to keep around, they should be placed in a
> different struct (with onfi in its name), and it should probably be
> dynamically allocated to not grow the nand_chip struct. I know
> allocating things in nand_scan_ident() is forbidden and that's why you
> stored things directly in nand_chip, but maybe we should address that
> problem instead of continuously find alternative solutions every time
> we need to allocate something from nand_scan_ident()...

I created a struct onfi_params that is static for now, but will be
dynamically allocated when the time will come. 

> 
> > +} __packed;  
> 
> I'm pretty sure __packed is not needed here.

Removed (I put it because nand_onfi_params and nand_jedec_params have
it).

> 
> > +
> >  /* The maximum expected count of bytes in the NAND ID sequence */
> >  #define NAND_MAX_ID_LEN 8
> >  
> > @@ -1161,10 +1175,6 @@ int nand_op_parser_exec_op(struct nand_chip *chip,
> >   *			non 0 if ONFI supported.
> >   * @jedec_version:	[INTERN] holds the chip JEDEC version (BCD encoded),
> >   *			non 0 if JEDEC supported.
> > - * @onfi_params:	[INTERN] holds the ONFI page parameter when ONFI is
> > - *			supported, 0 otherwise.
> > - * @jedec_params:	[INTERN] holds the JEDEC parameter page when JEDEC is
> > - *			supported, 0 otherwise.
> >   * @max_bb_per_die:	[INTERN] the max number of bad blocks each die of a
> >   *			this nand device will encounter their life times.
> >   * @blocks_per_die:	[INTERN] The number of PEBs in a die
> > @@ -1245,10 +1255,7 @@ struct nand_chip {
> >  	struct nand_id id;
> >  	int onfi_version;
> >  	int jedec_version;
> > -	union {
> > -		struct nand_onfi_params	onfi_params;
> > -		struct nand_jedec_params jedec_params;
> > -	};
> > +	struct nand_parameters parameters;
> >  	u16 max_bb_per_die;
> >  	u32 blocks_per_die;
> >  
> > @@ -1535,26 +1542,13 @@ struct platform_nand_data {
> >  	struct platform_nand_ctrl ctrl;
> >  };
> >  
> > -/* return the supported features. */
> > -static inline int onfi_feature(struct nand_chip *chip)
> > -{
> > -	return chip->onfi_version ? le16_to_cpu(chip->onfi_params.features) : 0;
> > -}
> > -
> >  /* return the supported asynchronous timing mode. */
> >  static inline int onfi_get_async_timing_mode(struct nand_chip *chip)
> >  {
> >  	if (!chip->onfi_version)
> >  		return ONFI_TIMING_MODE_UNKNOWN;
> > -	return le16_to_cpu(chip->onfi_params.async_timing_mode);
> > -}
> >  
> > -/* return the supported synchronous timing mode. */
> > -static inline int onfi_get_sync_timing_mode(struct nand_chip *chip)
> > -{
> > -	if (!chip->onfi_version)
> > -		return ONFI_TIMING_MODE_UNKNOWN;
> > -	return le16_to_cpu(chip->onfi_params.src_sync_timing_mode);
> > +	return chip->parameters.async_timing_mode;
> >  }
> >  
> >  int onfi_fill_data_interface(struct nand_chip *chip,
> > @@ -1591,13 +1585,6 @@ static inline int nand_opcode_8bits(unsigned int command)
> >  	return 0;
> >  }
> >  
> > -/* return the supported JEDEC features. */
> > -static inline int jedec_feature(struct nand_chip *chip)
> > -{
> > -	return chip->jedec_version ? le16_to_cpu(chip->jedec_params.features)
> > -		: 0;
> > -}
> > -
> >  /* get timing characteristics from ONFI timing mode. */
> >  const struct nand_sdr_timings *onfi_async_timing_mode_to_sdr_timings(int mode);
> >    
> 
> 
> 

Thanks,
Miquèl

-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 4/6] mtd: nand: Stop using a static parameter page for all chips

[Boris Brezillon]

On Sat,  3 Feb 2018 10:55:42 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index 3244f2594b6b..6d8667bb96f4 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -429,6 +429,20 @@ struct nand_jedec_params {
>  	__le16 crc;
>  } __packed;
>  
> +struct nand_parameters {
> +	/* Generic parameters */
> +	char model[20];
> +	/* ONFI parameters */
> +	bool support_setting_features;

It's not only about setting features, so maybe
'supports_set_get_features'.

> +	u16 t_prog;
> +	u16 t_bers;
> +	u16 t_r;
> +	u16 t_ccs;
> +	u16 async_timing_mode;
> +	u16 vendor_revision;
> +	u8 vendor[88];
> +} __packed;
> +




-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

[PATCH 5/6] mtd: nand: Allow vendors to declare (un)supported features

[Miquel Raynal]

From: Miquel Raynal <miquel.raynal@free-electrons.com>

If SET/GET_FEATURES is available (from the parameter page), use a
bitmap to declare what feature is actually supported.

Initialize the bitmap in the core to support timing changes (only
feature used by the core), also add support for Micron specific features
used in Micron initialization code (in the init routine).

Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 drivers/mtd/nand/nand_base.c   | 11 ++++++++---
 drivers/mtd/nand/nand_micron.c |  4 ++++
 include/linux/mtd/rawnand.h    |  5 ++++-
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 4a879b1635b3..859d9ba2678f 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1174,7 +1174,8 @@ int nand_get_features(struct nand_chip *chip, int addr,
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
 
-	if (!chip->parameters.support_setting_features)
+	if (!chip->parameters.support_setting_features ||
+	    !test_bit(addr, chip->parameters.feature_list))
 		return -ENOTSUPP;
 
 	return chip->onfi_get_features(mtd, chip, addr, subfeature_param);
@@ -1195,7 +1196,8 @@ int nand_set_features(struct nand_chip *chip, int addr,
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
 
-	if (!chip->parameters.support_setting_features)
+	if (!chip->parameters.support_setting_features ||
+	    !test_bit(addr, chip->parameters.feature_list))
 		return -ENOTSUPP;
 
 	return chip->onfi_set_features(mtd, chip, addr, subfeature_param);
@@ -5304,8 +5306,11 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
 	}
 
 	/* Save some parameters from the parameter page for future use */
-	if (le16_to_cpu(p->opt_cmd) & ONFI_OPT_CMD_SET_GET_FEATURES)
+	if (le16_to_cpu(p->opt_cmd) & ONFI_OPT_CMD_SET_GET_FEATURES) {
 		chip->parameters.support_setting_features = true;
+		bitmap_set(chip->parameters.feature_list,
+			   ONFI_FEATURE_ADDR_TIMING_MODE, 1);
+	}
 	chip->parameters.t_prog = le16_to_cpu(p->t_prog);
 	chip->parameters.t_bers = le16_to_cpu(p->t_bers);
 	chip->parameters.t_r = le16_to_cpu(p->t_r);
diff --git a/drivers/mtd/nand/nand_micron.c b/drivers/mtd/nand/nand_micron.c
index eaf14885e059..3c5b884e0eae 100644
--- a/drivers/mtd/nand/nand_micron.c
+++ b/drivers/mtd/nand/nand_micron.c
@@ -64,6 +64,10 @@ static int micron_nand_onfi_init(struct nand_chip *chip)
 		chip->setup_read_retry = micron_nand_setup_read_retry;
 	}
 
+	if (p->support_setting_features) {
+		set_bit(ONFI_FEATURE_ADDR_TIMING_MODE, p->feature_list);
+		set_bit(ONFI_FEATURE_ADDR_READ_RETRY, p->feature_list);
+	}
 
 	return 0;
 }
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index 6d8667bb96f4..39a38196dbac 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -21,6 +21,7 @@
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/flashchip.h>
 #include <linux/mtd/bbm.h>
+#include <linux/types.h>
 
 struct mtd_info;
 struct nand_flash_dev;
@@ -235,7 +236,8 @@ struct nand_chip;
 #define ONFI_TIMING_MODE_5		(1 << 5)
 #define ONFI_TIMING_MODE_UNKNOWN	(1 << 6)
 
-/* ONFI feature address */
+/* ONFI feature number/address */
+#define ONFI_FEATURE_NUMBER		256
 #define ONFI_FEATURE_ADDR_TIMING_MODE	0x1
 
 /* Vendor-specific feature address (Micron) */
@@ -434,6 +436,7 @@ struct nand_parameters {
 	char model[20];
 	/* ONFI parameters */
 	bool support_setting_features;
+	DECLARE_BITMAP(feature_list, ONFI_FEATURE_NUMBER);
 	u16 t_prog;
 	u16 t_bers;
 	u16 t_r;
-- 
2.14.1

Re: [PATCH 5/6] mtd: nand: Allow vendors to declare (un)supported features

[Boris Brezillon]

On Sat,  3 Feb 2018 10:55:43 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> From: Miquel Raynal <miquel.raynal@free-electrons.com>
> 
> If SET/GET_FEATURES is available (from the parameter page), use a
> bitmap to declare what feature is actually supported.
> 
> Initialize the bitmap in the core to support timing changes (only
> feature used by the core), also add support for Micron specific features
> used in Micron initialization code (in the init routine).
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> ---
>  drivers/mtd/nand/nand_base.c   | 11 ++++++++---
>  drivers/mtd/nand/nand_micron.c |  4 ++++
>  include/linux/mtd/rawnand.h    |  5 ++++-
>  3 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 4a879b1635b3..859d9ba2678f 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -1174,7 +1174,8 @@ int nand_get_features(struct nand_chip *chip, int addr,
>  {
>  	struct mtd_info *mtd = nand_to_mtd(chip);
>  
> -	if (!chip->parameters.support_setting_features)
> +	if (!chip->parameters.support_setting_features ||
> +	    !test_bit(addr, chip->parameters.feature_list))
>  		return -ENOTSUPP;
>  
>  	return chip->onfi_get_features(mtd, chip, addr, subfeature_param);
> @@ -1195,7 +1196,8 @@ int nand_set_features(struct nand_chip *chip, int addr,
>  {
>  	struct mtd_info *mtd = nand_to_mtd(chip);
>  
> -	if (!chip->parameters.support_setting_features)
> +	if (!chip->parameters.support_setting_features ||
> +	    !test_bit(addr, chip->parameters.feature_list))
>  		return -ENOTSUPP;
>  
>  	return chip->onfi_set_features(mtd, chip, addr, subfeature_param);
> @@ -5304,8 +5306,11 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
>  	}
>  
>  	/* Save some parameters from the parameter page for future use */
> -	if (le16_to_cpu(p->opt_cmd) & ONFI_OPT_CMD_SET_GET_FEATURES)
> +	if (le16_to_cpu(p->opt_cmd) & ONFI_OPT_CMD_SET_GET_FEATURES) {
>  		chip->parameters.support_setting_features = true;
> +		bitmap_set(chip->parameters.feature_list,
> +			   ONFI_FEATURE_ADDR_TIMING_MODE, 1);
> +	}
>  	chip->parameters.t_prog = le16_to_cpu(p->t_prog);
>  	chip->parameters.t_bers = le16_to_cpu(p->t_bers);
>  	chip->parameters.t_r = le16_to_cpu(p->t_r);
> diff --git a/drivers/mtd/nand/nand_micron.c b/drivers/mtd/nand/nand_micron.c
> index eaf14885e059..3c5b884e0eae 100644
> --- a/drivers/mtd/nand/nand_micron.c
> +++ b/drivers/mtd/nand/nand_micron.c
> @@ -64,6 +64,10 @@ static int micron_nand_onfi_init(struct nand_chip *chip)
>  		chip->setup_read_retry = micron_nand_setup_read_retry;
>  	}
>  
> +	if (p->support_setting_features) {
> +		set_bit(ONFI_FEATURE_ADDR_TIMING_MODE, p->feature_list);

No need to set it again since it's already set by default.

> +		set_bit(ONFI_FEATURE_ADDR_READ_RETRY, p->feature_list);
> +	}
>  
>  	return 0;
>  }
> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index 6d8667bb96f4..39a38196dbac 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -21,6 +21,7 @@
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/flashchip.h>
>  #include <linux/mtd/bbm.h>
> +#include <linux/types.h>
>  
>  struct mtd_info;
>  struct nand_flash_dev;
> @@ -235,7 +236,8 @@ struct nand_chip;
>  #define ONFI_TIMING_MODE_5		(1 << 5)
>  #define ONFI_TIMING_MODE_UNKNOWN	(1 << 6)
>  
> -/* ONFI feature address */
> +/* ONFI feature number/address */
> +#define ONFI_FEATURE_NUMBER		256
>  #define ONFI_FEATURE_ADDR_TIMING_MODE	0x1
>  
>  /* Vendor-specific feature address (Micron) */
> @@ -434,6 +436,7 @@ struct nand_parameters {
>  	char model[20];
>  	/* ONFI parameters */
>  	bool support_setting_features;
> +	DECLARE_BITMAP(feature_list, ONFI_FEATURE_NUMBER);

This is not ONFI specific. The SET/GET_FEATURES are supported by JEDEC
and also non-JEDEC/ONFI chips.

Could we move that to the generic section, or maybe declare a
nand_features struct that you could place directly in nand_chip:

struct nand_features {
	DECLARE_BITMAP(supported, ONFI_FEATURE_NUMBER);
};

BTW, are we sure that all features can both be set and retrieved? If
that's not the case, then maybe we should have a bitmap for each
operation (set and get).

>  	u16 t_prog;
>  	u16 t_bers;
>  	u16 t_r;



-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH 5/6] mtd: nand: Allow vendors to declare (un)supported features

[Miquel Raynal]

Hi Boris,

On Wed, 14 Feb 2018 09:51:37 +0100, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:

> On Sat,  3 Feb 2018 10:55:43 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > From: Miquel Raynal <miquel.raynal@free-electrons.com>
> > 
> > If SET/GET_FEATURES is available (from the parameter page), use a
> > bitmap to declare what feature is actually supported.
> > 
> > Initialize the bitmap in the core to support timing changes (only
> > feature used by the core), also add support for Micron specific features
> > used in Micron initialization code (in the init routine).
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> > ---
> >  drivers/mtd/nand/nand_base.c   | 11 ++++++++---
> >  drivers/mtd/nand/nand_micron.c |  4 ++++
> >  include/linux/mtd/rawnand.h    |  5 ++++-
> >  3 files changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index 4a879b1635b3..859d9ba2678f 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -1174,7 +1174,8 @@ int nand_get_features(struct nand_chip *chip, int addr,
> >  {
> >  	struct mtd_info *mtd = nand_to_mtd(chip);
> >  
> > -	if (!chip->parameters.support_setting_features)
> > +	if (!chip->parameters.support_setting_features ||
> > +	    !test_bit(addr, chip->parameters.feature_list))
> >  		return -ENOTSUPP;
> >  
> >  	return chip->onfi_get_features(mtd, chip, addr, subfeature_param);
> > @@ -1195,7 +1196,8 @@ int nand_set_features(struct nand_chip *chip, int addr,
> >  {
> >  	struct mtd_info *mtd = nand_to_mtd(chip);
> >  
> > -	if (!chip->parameters.support_setting_features)
> > +	if (!chip->parameters.support_setting_features ||
> > +	    !test_bit(addr, chip->parameters.feature_list))
> >  		return -ENOTSUPP;
> >  
> >  	return chip->onfi_set_features(mtd, chip, addr, subfeature_param);
> > @@ -5304,8 +5306,11 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
> >  	}
> >  
> >  	/* Save some parameters from the parameter page for future use */
> > -	if (le16_to_cpu(p->opt_cmd) & ONFI_OPT_CMD_SET_GET_FEATURES)
> > +	if (le16_to_cpu(p->opt_cmd) & ONFI_OPT_CMD_SET_GET_FEATURES) {
> >  		chip->parameters.support_setting_features = true;
> > +		bitmap_set(chip->parameters.feature_list,
> > +			   ONFI_FEATURE_ADDR_TIMING_MODE, 1);
> > +	}
> >  	chip->parameters.t_prog = le16_to_cpu(p->t_prog);
> >  	chip->parameters.t_bers = le16_to_cpu(p->t_bers);
> >  	chip->parameters.t_r = le16_to_cpu(p->t_r);
> > diff --git a/drivers/mtd/nand/nand_micron.c b/drivers/mtd/nand/nand_micron.c
> > index eaf14885e059..3c5b884e0eae 100644
> > --- a/drivers/mtd/nand/nand_micron.c
> > +++ b/drivers/mtd/nand/nand_micron.c
> > @@ -64,6 +64,10 @@ static int micron_nand_onfi_init(struct nand_chip *chip)
> >  		chip->setup_read_retry = micron_nand_setup_read_retry;
> >  	}
> >  
> > +	if (p->support_setting_features) {
> > +		set_bit(ONFI_FEATURE_ADDR_TIMING_MODE, p->feature_list);  
> 
> No need to set it again since it's already set by default.

Good catch. Removed.

> 
> > +		set_bit(ONFI_FEATURE_ADDR_READ_RETRY, p->feature_list);
> > +	}
> >  
> >  	return 0;
> >  }
> > diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> > index 6d8667bb96f4..39a38196dbac 100644
> > --- a/include/linux/mtd/rawnand.h
> > +++ b/include/linux/mtd/rawnand.h
> > @@ -21,6 +21,7 @@
> >  #include <linux/mtd/mtd.h>
> >  #include <linux/mtd/flashchip.h>
> >  #include <linux/mtd/bbm.h>
> > +#include <linux/types.h>
> >  
> >  struct mtd_info;
> >  struct nand_flash_dev;
> > @@ -235,7 +236,8 @@ struct nand_chip;
> >  #define ONFI_TIMING_MODE_5		(1 << 5)
> >  #define ONFI_TIMING_MODE_UNKNOWN	(1 << 6)
> >  
> > -/* ONFI feature address */
> > +/* ONFI feature number/address */
> > +#define ONFI_FEATURE_NUMBER		256
> >  #define ONFI_FEATURE_ADDR_TIMING_MODE	0x1
> >  
> >  /* Vendor-specific feature address (Micron) */
> > @@ -434,6 +436,7 @@ struct nand_parameters {
> >  	char model[20];
> >  	/* ONFI parameters */
> >  	bool support_setting_features;
> > +	DECLARE_BITMAP(feature_list, ONFI_FEATURE_NUMBER);  
> 
> This is not ONFI specific. The SET/GET_FEATURES are supported by JEDEC
> and also non-JEDEC/ONFI chips.

Moved in the "generic" section, together with the
support_get_set_features (ex. support_setting_features).

> 
> Could we move that to the generic section, or maybe declare a
> nand_features struct that you could place directly in nand_chip:
> 
> struct nand_features {
> 	DECLARE_BITMAP(supported, ONFI_FEATURE_NUMBER);
> };
> 
> BTW, are we sure that all features can both be set and retrieved? If
> that's not the case, then maybe we should have a bitmap for each
> operation (set and get).

Actually, no. I added a second bitmap for that.

> 
> >  	u16 t_prog;
> >  	u16 t_bers;
> >  	u16 t_r;  
> 
> 
> 



-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH 6/6] mtd: nand: macronix: Unflag the support of changing timings for MX30LF2G18AC

[Miquel Raynal]

From: Miquel Raynal <miquel.raynal@free-electrons.com>

The MX30LF2G18AC chip declares in its parameter page supporting
SET/GET_FEATURES but when it comes to timings, experience shows that it
is not the case.

Unflag this feature for this particular chip in the nand_parameters
structure to avoid unnecessary errors and downturns.

Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 drivers/mtd/nand/nand_macronix.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/mtd/nand/nand_macronix.c b/drivers/mtd/nand/nand_macronix.c
index d290ff2a6d2f..c58567834595 100644
--- a/drivers/mtd/nand/nand_macronix.c
+++ b/drivers/mtd/nand/nand_macronix.c
@@ -19,9 +19,21 @@
 
 static int macronix_nand_init(struct nand_chip *chip)
 {
+	const char model[20] = "MX30LF2G18AC";
+
 	if (nand_is_slc(chip))
 		chip->bbt_options |= NAND_BBT_SCAN2NDPAGE;
 
+	/*
+	 * MX30LF2G18AC chip does not support using SET/GET_FEATURES to change
+	 * the timings unlike what is declared in the parameter page. Unflag
+	 * this feature to avoid unnecessary downturns.
+	 */
+	if (chip->parameters.support_setting_features &&
+	    !strncmp(model, chip->parameters.model, ARRAY_SIZE(model)))
+		bitmap_clear(chip->parameters.feature_list,
+			     ONFI_FEATURE_ADDR_TIMING_MODE, 1);
+
 	return 0;
 }
 
-- 
2.14.1

Re: [PATCH 6/6] mtd: nand: macronix: Unflag the support of changing timings for MX30LF2G18AC

[Boris Brezillon]

On Sat,  3 Feb 2018 10:55:44 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> From: Miquel Raynal <miquel.raynal@free-electrons.com>
> 
> The MX30LF2G18AC chip declares in its parameter page supporting
> SET/GET_FEATURES but when it comes to timings, experience shows that it
> is not the case.
> 
> Unflag this feature for this particular chip in the nand_parameters
> structure to avoid unnecessary errors and downturns.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> ---
>  drivers/mtd/nand/nand_macronix.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/mtd/nand/nand_macronix.c b/drivers/mtd/nand/nand_macronix.c
> index d290ff2a6d2f..c58567834595 100644
> --- a/drivers/mtd/nand/nand_macronix.c
> +++ b/drivers/mtd/nand/nand_macronix.c
> @@ -19,9 +19,21 @@
>  
>  static int macronix_nand_init(struct nand_chip *chip)
>  {
> +	const char model[20] = "MX30LF2G18AC";
> +
>  	if (nand_is_slc(chip))
>  		chip->bbt_options |= NAND_BBT_SCAN2NDPAGE;
>  
> +	/*
> +	 * MX30LF2G18AC chip does not support using SET/GET_FEATURES to change
> +	 * the timings unlike what is declared in the parameter page. Unflag
> +	 * this feature to avoid unnecessary downturns.
> +	 */
> +	if (chip->parameters.support_setting_features &&
> +	    !strncmp(model, chip->parameters.model, ARRAY_SIZE(model)))

Why not

	    !strcmp("MX30LF2G18AC", chip->parameters.model)

?

> +		bitmap_clear(chip->parameters.feature_list,
> +			     ONFI_FEATURE_ADDR_TIMING_MODE, 1);
> +
>  	return 0;
>  }
>  



-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com