[PATCH v3 07/14] mtd: rawnand: use wrappers to call onfi GET/SET_FEATURES
Boris Brezillon
boris.brezillon at bootlin.com
Fri Mar 2 14:40:54 PST 2018
On Fri, 2 Mar 2018 15:24:15 +0100
Miquel Raynal <miquel.raynal at bootlin.com> wrote:
> 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 ->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).
>
Why don't you move this patch just after patch 3 so that you can use
the new wrapper in patch 5?
> Signed-off-by: Miquel Raynal <miquel.raynal at bootlin.com>
> ---
> drivers/mtd/nand/raw/nand_base.c | 127 ++++++++++++++++++++++++-------------
> drivers/mtd/nand/raw/nand_micron.c | 18 +++---
> include/linux/mtd/rawnand.h | 3 +
> 3 files changed, 96 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 70b59672362c..e5bcfbf7c7f6 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/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->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->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->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->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);
> @@ -4820,11 +4869,6 @@ static int nand_default_set_features(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);
> }
>
> @@ -4839,11 +4883,6 @@ static int nand_default_get_features(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);
> }
>
> diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
> index ab3e3a1a5212..b825656f6284 100644
> --- a/drivers/mtd/nand/raw/nand_micron.c
> +++ b/drivers/mtd/nand/raw/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->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->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,10 @@ static int micron_supports_on_die_ecc(struct nand_chip *chip)
> if (ret)
> return MICRON_ON_DIE_UNSUPPORTED;
>
> - chip->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 +228,10 @@ static int micron_supports_on_die_ecc(struct nand_chip *chip)
> if (ret)
> return MICRON_ON_DIE_UNSUPPORTED;
>
> - chip->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 fb2e288ef8b1..3cc2a3435b20 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_get_set_features_notsupp(struct mtd_info *mtd, struct nand_chip *chip,
> int addr, u8 *subfeature_param);
--
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
More information about the linux-mtd
mailing list