| Message ID | 20180203095544.9855-3-miquel.raynal@bootlin.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Boris Brezillon |
| Headers | show |
| Series | Improve timings handling in the NAND framework | expand |
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
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
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,