* [PATCH 0/2] mtd: nand: automate NAND timings selection
@ 2015-10-23 11:03 Boris Brezillon
2015-10-23 11:03 ` [PATCH 1/2] " Boris Brezillon
2015-10-23 11:03 ` [PATCH 2/2] mtd: nand: sunxi: switch from manual to automated timing config Boris Brezillon
0 siblings, 2 replies; 5+ messages in thread
From: Boris Brezillon @ 2015-10-23 11:03 UTC (permalink / raw)
To: David Woodhouse, Brian Norris, linux-mtd
Cc: linux-kernel, Maxime Ripard, linux-sunxi, Boris Brezillon
Hello,
This series aims at automating the NAND timings selection which is
currently supposed to be done in each NAND controller driver, thus
simplifying drivers implementation.
Patch 1 also opens the door to DDR NAND support, though setting DDR
timings is currently not supported.
Note that patch 2 is given here as an example of how patch 1 simplifies
things, but won't apply cleanly on mtd-next (it's based on top of other
changes I haven't submitted yet).
Best Regards,
Boris
Boris Brezillon (2):
mtd: nand: automate NAND timings selection
mtd: nand: sunxi: switch from manual to automated timing config
drivers/mtd/nand/nand_base.c | 189 +++++++++++++++++++++++++++++++++++++++++-
drivers/mtd/nand/sunxi_nand.c | 72 ++++------------
include/linux/mtd/nand.h | 115 ++++++++++++++-----------
3 files changed, 270 insertions(+), 106 deletions(-)
--
2.1.4
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] mtd: nand: automate NAND timings selection
2015-10-23 11:03 [PATCH 0/2] mtd: nand: automate NAND timings selection Boris Brezillon
@ 2015-10-23 11:03 ` Boris Brezillon
2015-11-02 0:37 ` Ezequiel Garcia
2015-10-23 11:03 ` [PATCH 2/2] mtd: nand: sunxi: switch from manual to automated timing config Boris Brezillon
1 sibling, 1 reply; 5+ messages in thread
From: Boris Brezillon @ 2015-10-23 11:03 UTC (permalink / raw)
To: David Woodhouse, Brian Norris, linux-mtd
Cc: linux-kernel, Maxime Ripard, linux-sunxi, Boris Brezillon
The NAND framework provides several helpers to query timing modes supported
by a NAND chip, but this implies that all NAND controller drivers have
to implement the same timings selection dance.
Provide a common logic to select the best timings based on ONFI or
->onfi_timing_mode_default information.
NAND controller willing to support timings adjustment should just
implement the ->setup_data_interface() method.
Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
drivers/mtd/nand/nand_base.c | 189 ++++++++++++++++++++++++++++++++++++++++++-
include/linux/mtd/nand.h | 115 +++++++++++++++-----------
2 files changed, 254 insertions(+), 50 deletions(-)
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index eaf1fcd..52a1f89 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3323,6 +3323,144 @@ static void nand_onfi_detect_micron(struct nand_chip *chip,
chip->setup_read_retry = nand_setup_read_retry_micron;
}
+/**
+ * nand_setup_data_interface - Setup the data interface and timings on the
+ * controller side
+ * @mtd: MTD device structure
+ * @conf: new configuration to apply
+ *
+ * Try to configure the NAND controller to support the provided data
+ * interface configuration.
+ *
+ * Returns 0 in case of success or -ERROR_CODE.
+ */
+static int nand_setup_data_interface(struct mtd_info *mtd,
+ const struct nand_data_interface *conf)
+{
+ struct nand_chip *chip = mtd->priv;
+ int ret;
+
+ if (!chip->setup_data_interface)
+ return -ENOTSUPP;
+
+ ret = chip->setup_data_interface(mtd, conf, false);
+ if (ret)
+ return ret;
+
+ *chip->data_iface = *conf;
+
+ return 0;
+}
+
+/**
+ * nand_setup_data_interface - Check if a data interface config is supported
+ * by the NAND controller
+ * @mtd: MTD device structure
+ * @conf: new configuration to apply
+ *
+ * Check if the provided data interface configuration is supported by the
+ * NAND controller.
+ *
+ * Returns 0 if it is supported or -ERROR_CODE.
+ */
+static int nand_check_data_interface(struct mtd_info *mtd,
+ const struct nand_data_interface *conf)
+{
+ struct nand_chip *chip = mtd->priv;
+
+ if (!chip->setup_data_interface)
+ return -ENOTSUPP;
+
+ return chip->setup_data_interface(mtd, conf, true);
+}
+
+/**
+ * nand_configure_data_interface - Configure the data interface and timings
+ * @mtd: MTD device structure
+ *
+ * Try to configure the data interface and NAND timings appropriately.
+ * First tries to retrieve supported timing modes from ONFI information,
+ * and if the NAND chip does not support ONFI, relies on the
+ * ->onfi_timing_mode_default specified in the nand_ids table.
+ *
+ * Returns 0 in case of success or -ERROR_CODE.
+ */
+static int nand_configure_data_interface(struct mtd_info *mtd)
+{
+ struct nand_chip *chip = mtd->priv;
+ struct nand_data_interface *conf;
+ int modes, mode, ret = -EINVAL;
+
+ conf = kzalloc(sizeof(*conf), GFP_KERNEL);
+ if (!conf)
+ return -ENOMEM;
+
+ /* TODO: support DDR interfaces */
+ conf->type = NAND_SDR_IFACE;
+
+ /*
+ * First try to identify the best timings from ONFI parameters and
+ * if the NAND does not support ONFI, fallback to the default ONFI
+ * timing mode.
+ */
+ modes = onfi_get_async_timing_mode(chip);
+ if (modes != ONFI_TIMING_MODE_UNKNOWN) {
+ for (mode = fls(modes) - 1; mode >= 0; mode--) {
+ conf->timings.sdr =
+ *onfi_async_timing_mode_to_sdr_timings(mode);
+
+ ret = nand_check_data_interface(mtd, conf);
+ if (!ret)
+ break;
+ }
+ } else {
+ mode = chip->onfi_timing_mode_default;
+ conf->timings.sdr =
+ *onfi_async_timing_mode_to_sdr_timings(mode);
+
+ ret = nand_check_data_interface(mtd, conf);
+ }
+
+ if (!ret) {
+ uint8_t tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = { mode };
+ int i;
+
+ /*
+ * Ensure the timing mode has be changed on the chip side
+ * before changing timings on the controller side.
+ */
+ if (modes != ONFI_TIMING_MODE_UNKNOWN) {
+ /*
+ * FIXME: should we really set the timing mode on all
+ * dies?
+ */
+ for (i = 0; i < chip->numchips; i++) {
+ chip->select_chip(mtd, i);
+ ret = chip->onfi_set_features(mtd, chip,
+ ONFI_FEATURE_ADDR_TIMING_MODE,
+ tmode_param);
+ }
+ chip->select_chip(mtd, -1);
+ }
+
+ if (!ret) {
+ ret = nand_setup_data_interface(mtd, conf);
+
+ /*
+ * Reset the NAND chip if the data interface setup
+ * failed so that the chip goes back to a known state
+ * (timing mode 0).
+ */
+ if (ret)
+ chip->cmdfunc(mtd, NAND_CMD_RESET, -1, -1);
+ }
+ }
+
+ kfree(conf);
+
+ return ret;
+}
+
/*
* Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
*/
@@ -3906,6 +4044,11 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
chip->options &= ~NAND_SAMSUNG_LP_OPTIONS;
ident_done:
+ /*
+ * Setup the NAND interface (interface type + timings).
+ */
+ nand_configure_data_interface(mtd);
+
/* Try to identify manufacturer */
for (maf_idx = 0; nand_manuf_ids[maf_idx].id != 0x0; maf_idx++) {
if (nand_manuf_ids[maf_idx].id == *maf_id)
@@ -4033,6 +4176,41 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
/* Set the default functions */
nand_set_defaults(chip, chip->options & NAND_BUSWIDTH_16);
+ /*
+ * Allocate an interface config struct if the controller implements the
+ * ->apply_interface_conf() method.
+ */
+ if (chip->setup_data_interface) {
+ chip->data_iface = kzalloc(sizeof(*chip->data_iface),
+ GFP_KERNEL);
+ if (!chip->data_iface)
+ return -ENOMEM;
+
+ /*
+ * The ONFI specification says:
+ * "
+ * To transition from NV-DDR or NV-DDR2 to the SDR data
+ * interface, the host shall use the Reset (FFh) command
+ * using SDR timing mode 0. A device in any timing mode is
+ * required to recognize Reset (FFh) command issued in SDR
+ * timing mode 0.
+ * "
+ *
+ * Configure the data interface in SDR mode and set the
+ * timings to timing mode 0. The Reset command is issued
+ * in nand_get_flash_type().
+ */
+
+ chip->data_iface->type = NAND_SDR_IFACE;
+ chip->data_iface->timings.sdr =
+ *onfi_async_timing_mode_to_sdr_timings(0);
+ ret = chip->setup_data_interface(mtd, chip->data_iface, false);
+ if (ret) {
+ pr_err("Failed to configure data interface to SDR timing mode 0\n");
+ goto err;
+ }
+ }
+
/* Read the flash type */
type = nand_get_flash_type(mtd, chip, &nand_maf_id,
&nand_dev_id, table);
@@ -4041,7 +4219,9 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
if (!(chip->options & NAND_SCAN_SILENT_NODEV))
pr_warn("No NAND device found\n");
chip->select_chip(mtd, -1);
- return PTR_ERR(type);
+ kfree(chip->data_iface);
+ ret = PTR_ERR(type);
+ goto err;
}
chip->select_chip(mtd, -1);
@@ -4069,6 +4249,10 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
mtd->size = i * chip->chipsize;
return 0;
+
+err:
+ kfree(chip->data_iface);
+ return ret;
}
EXPORT_SYMBOL(nand_scan_ident);
@@ -4476,6 +4660,9 @@ void nand_release(struct mtd_info *mtd)
mtd_device_unregister(mtd);
+ /* Free interface config struct */
+ kfree(chip->data_iface);
+
/* Free bad block table memory */
kfree(chip->bbt);
if (!(chip->options & NAND_OWN_BUFFERS))
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 7ebd449..aee4c1e 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -558,6 +558,66 @@ struct nand_buffers {
uint8_t *databuf;
};
+/*
+ * struct nand_sdr_timings - SDR NAND chip timings
+ *
+ * This struct defines the timing requirements of a SDR NAND chip.
+ * These information can be found in every NAND datasheets and the timings
+ * meaning are described in the ONFI specifications:
+ * www.onfi.org/~/media/ONFI/specs/onfi_3_1_spec.pdf (chapter 4.15 Timing
+ * Parameters)
+ *
+ * All these timings are expressed in picoseconds.
+ */
+
+struct nand_sdr_timings {
+ u32 tALH_min;
+ u32 tADL_min;
+ u32 tALS_min;
+ u32 tAR_min;
+ u32 tCEA_max;
+ u32 tCEH_min;
+ u32 tCH_min;
+ u32 tCHZ_max;
+ u32 tCLH_min;
+ u32 tCLR_min;
+ u32 tCLS_min;
+ u32 tCOH_min;
+ u32 tCS_min;
+ u32 tDH_min;
+ u32 tDS_min;
+ u32 tFEAT_max;
+ u32 tIR_min;
+ u32 tITC_max;
+ u32 tRC_min;
+ u32 tREA_max;
+ u32 tREH_min;
+ u32 tRHOH_min;
+ u32 tRHW_min;
+ u32 tRHZ_max;
+ u32 tRLOH_min;
+ u32 tRP_min;
+ u32 tRR_min;
+ u64 tRST_max;
+ u32 tWB_max;
+ u32 tWC_min;
+ u32 tWH_min;
+ u32 tWHR_min;
+ u32 tWP_min;
+ u32 tWW_min;
+};
+
+enum nand_data_interface_type {
+ NAND_SDR_IFACE,
+};
+
+struct nand_data_interface {
+ enum nand_data_interface_type type;
+ union {
+ struct nand_sdr_timings sdr;
+ } timings;
+};
+
/**
* struct nand_chip - NAND Private Flash Chip Data
* @IO_ADDR_R: [BOARDSPECIFIC] address to read the 8 I/O lines of the
@@ -690,6 +750,10 @@ struct nand_chip {
int (*onfi_get_features)(struct mtd_info *mtd, struct nand_chip *chip,
int feature_addr, uint8_t *subfeature_para);
int (*setup_read_retry)(struct mtd_info *mtd, int retry_mode);
+ int (*setup_data_interface)(struct mtd_info *mtd,
+ const struct nand_data_interface *conf,
+ bool check_only);
+
int chip_delay;
unsigned int options;
@@ -719,6 +783,8 @@ struct nand_chip {
struct nand_jedec_params jedec_params;
};
+ struct nand_data_interface *data_iface;
+
int read_retries;
flstate_t state;
@@ -993,55 +1059,6 @@ static inline int jedec_feature(struct nand_chip *chip)
: 0;
}
-/*
- * struct nand_sdr_timings - SDR NAND chip timings
- *
- * This struct defines the timing requirements of a SDR NAND chip.
- * These informations can be found in every NAND datasheets and the timings
- * meaning are described in the ONFI specifications:
- * www.onfi.org/~/media/ONFI/specs/onfi_3_1_spec.pdf (chapter 4.15 Timing
- * Parameters)
- *
- * All these timings are expressed in picoseconds.
- */
-
-struct nand_sdr_timings {
- u32 tALH_min;
- u32 tADL_min;
- u32 tALS_min;
- u32 tAR_min;
- u32 tCEA_max;
- u32 tCEH_min;
- u32 tCH_min;
- u32 tCHZ_max;
- u32 tCLH_min;
- u32 tCLR_min;
- u32 tCLS_min;
- u32 tCOH_min;
- u32 tCS_min;
- u32 tDH_min;
- u32 tDS_min;
- u32 tFEAT_max;
- u32 tIR_min;
- u32 tITC_max;
- u32 tRC_min;
- u32 tREA_max;
- u32 tREH_min;
- u32 tRHOH_min;
- u32 tRHW_min;
- u32 tRHZ_max;
- u32 tRLOH_min;
- u32 tRP_min;
- u32 tRR_min;
- u64 tRST_max;
- u32 tWB_max;
- u32 tWC_min;
- u32 tWH_min;
- u32 tWHR_min;
- u32 tWP_min;
- u32 tWW_min;
-};
-
/* get timing characteristics from ONFI timing mode. */
const struct nand_sdr_timings *onfi_async_timing_mode_to_sdr_timings(int mode);
--
2.1.4
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] mtd: nand: sunxi: switch from manual to automated timing config
2015-10-23 11:03 [PATCH 0/2] mtd: nand: automate NAND timings selection Boris Brezillon
2015-10-23 11:03 ` [PATCH 1/2] " Boris Brezillon
@ 2015-10-23 11:03 ` Boris Brezillon
1 sibling, 0 replies; 5+ messages in thread
From: Boris Brezillon @ 2015-10-23 11:03 UTC (permalink / raw)
To: David Woodhouse, Brian Norris, linux-mtd
Cc: linux-kernel, Maxime Ripard, linux-sunxi, Boris Brezillon
The NAND framework is now able to select the best NAND timings for us.
All we have to do is implement a ->setup_data_interface() function to
apply those timings and remove the timing selection code from the sunxi
driver.
Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
drivers/mtd/nand/sunxi_nand.c | 72 ++++++++++---------------------------------
1 file changed, 16 insertions(+), 56 deletions(-)
diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c
index b657235..b6f21f4 100644
--- a/drivers/mtd/nand/sunxi_nand.c
+++ b/drivers/mtd/nand/sunxi_nand.c
@@ -1131,7 +1131,8 @@ static int _sunxi_nand_lookup_timing(const s32 *lut, int lut_size, u32 duration,
_sunxi_nand_lookup_timing(l, ARRAY_SIZE(l), p, c)
static int sunxi_nand_chip_set_timings(struct sunxi_nand_chip *chip,
- const struct nand_sdr_timings *timings)
+ const struct nand_sdr_timings *timings,
+ bool check_only)
{
struct sunxi_nfc *nfc = to_sunxi_nfc(chip->nand.controller);
u32 min_clk_period = 0;
@@ -1224,6 +1225,9 @@ static int sunxi_nand_chip_set_timings(struct sunxi_nand_chip *chip,
return tRHW;
}
+ if (check_only)
+ return 0;
+
/*
* TODO: according to ONFI specs this value only applies for DDR NAND,
* but Allwinner seems to set this to 0x7. Mimic them for now.
@@ -1254,42 +1258,19 @@ static int sunxi_nand_chip_set_timings(struct sunxi_nand_chip *chip,
return 0;
}
-static int sunxi_nand_chip_init_timings(struct sunxi_nand_chip *chip,
- struct device_node *np)
+static int sunxi_nand_setup_data_interface(struct mtd_info *mtd,
+ const struct nand_data_interface *conf,
+ bool check_only)
{
- const struct nand_sdr_timings *timings;
- int ret;
- int mode;
-
- mode = onfi_get_async_timing_mode(&chip->nand);
- if (mode == ONFI_TIMING_MODE_UNKNOWN) {
- mode = chip->nand.onfi_timing_mode_default;
- } else {
- uint8_t feature[ONFI_SUBFEATURE_PARAM_LEN] = {};
- int i;
-
- mode = fls(mode) - 1;
- if (mode < 0)
- mode = 0;
-
- feature[0] = mode;
- for (i = 0; i < chip->nsels; i++) {
- chip->nand.select_chip(&chip->mtd, i);
- ret = chip->nand.onfi_set_features(&chip->mtd,
- &chip->nand,
- ONFI_FEATURE_ADDR_TIMING_MODE,
- feature);
- chip->nand.select_chip(&chip->mtd, -1);
- if (ret)
- return ret;
- }
- }
+ struct nand_chip *nand = mtd->priv;
+ struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
- timings = onfi_async_timing_mode_to_sdr_timings(mode);
- if (IS_ERR(timings))
- return PTR_ERR(timings);
+ /* TODO: add support for DDR NANDs */
+ if (conf->type != NAND_SDR_IFACE)
+ return -ENOTSUPP;
- return sunxi_nand_chip_set_timings(chip, timings);
+ return sunxi_nand_chip_set_timings(sunxi_nand, &conf->timings.sdr,
+ check_only);
}
static int sunxi_nand_hw_common_ecc_ctrl_init(struct mtd_info *mtd,
@@ -1502,7 +1483,6 @@ static int sunxi_nand_ecc_init(struct mtd_info *mtd, struct nand_ecc_ctrl *ecc,
static int sunxi_nand_chip_init(struct device *dev, struct sunxi_nfc *nfc,
struct device_node *np)
{
- const struct nand_sdr_timings *timings;
struct sunxi_nand_chip *chip;
struct mtd_part_parser_data ppdata;
struct mtd_info *mtd;
@@ -1578,21 +1558,6 @@ static int sunxi_nand_chip_init(struct device *dev, struct sunxi_nfc *nfc,
}
}
- timings = onfi_async_timing_mode_to_sdr_timings(0);
- if (IS_ERR(timings)) {
- ret = PTR_ERR(timings);
- dev_err(dev,
- "could not retrieve timings for ONFI mode 0: %d\n",
- ret);
- return ret;
- }
-
- ret = sunxi_nand_chip_set_timings(chip, timings);
- if (ret) {
- dev_err(dev, "could not configure chip timings: %d\n", ret);
- return ret;
- }
-
nand = &chip->nand;
/* Default tR value specified in the ONFI spec (chapter 4.15.1) */
nand->chip_delay = 200;
@@ -1608,6 +1573,7 @@ static int sunxi_nand_chip_init(struct device *dev, struct sunxi_nfc *nfc,
nand->read_buf = sunxi_nfc_read_buf;
nand->write_buf = sunxi_nfc_write_buf;
nand->read_byte = sunxi_nfc_read_byte;
+ nand->setup_data_interface = sunxi_nand_setup_data_interface;
mtd = &chip->mtd;
mtd->dev.parent = dev;
@@ -1625,12 +1591,6 @@ static int sunxi_nand_chip_init(struct device *dev, struct sunxi_nfc *nfc,
nand->bbt_options |= NAND_BBT_SCANRAWMODE;
}
- ret = sunxi_nand_chip_init_timings(chip, np);
- if (ret) {
- dev_err(dev, "could not configure chip timings: %d\n", ret);
- return ret;
- }
-
ret = sunxi_nand_ecc_init(mtd, &nand->ecc, np);
if (ret) {
dev_err(dev, "ECC init failed: %d\n", ret);
--
2.1.4
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] mtd: nand: automate NAND timings selection
2015-10-23 11:03 ` [PATCH 1/2] " Boris Brezillon
@ 2015-11-02 0:37 ` Ezequiel Garcia
2015-11-02 8:39 ` Boris Brezillon
0 siblings, 1 reply; 5+ messages in thread
From: Ezequiel Garcia @ 2015-11-02 0:37 UTC (permalink / raw)
To: Boris Brezillon
Cc: David Woodhouse, Brian Norris, linux-mtd, Maxime Ripard,
linux-sunxi, linux-kernel
Hi Boris,
This looks nice. I gave a try at this patch and it allows to simplify
pxa3xx-nand greatly. A few comments below.
On 23 Oct 01:03 PM, Boris Brezillon wrote:
> The NAND framework provides several helpers to query timing modes supported
> by a NAND chip, but this implies that all NAND controller drivers have
> to implement the same timings selection dance.
>
> Provide a common logic to select the best timings based on ONFI or
> ->onfi_timing_mode_default information.
> NAND controller willing to support timings adjustment should just
> implement the ->setup_data_interface() method.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> drivers/mtd/nand/nand_base.c | 189 ++++++++++++++++++++++++++++++++++++++++++-
> include/linux/mtd/nand.h | 115 +++++++++++++++-----------
> 2 files changed, 254 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index eaf1fcd..52a1f89 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3323,6 +3323,144 @@ static void nand_onfi_detect_micron(struct nand_chip *chip,
> chip->setup_read_retry = nand_setup_read_retry_micron;
> }
>
> +/**
> + * nand_setup_data_interface - Setup the data interface and timings on the
> + * controller side
> + * @mtd: MTD device structure
> + * @conf: new configuration to apply
> + *
> + * Try to configure the NAND controller to support the provided data
> + * interface configuration.
> + *
> + * Returns 0 in case of success or -ERROR_CODE.
> + */
> +static int nand_setup_data_interface(struct mtd_info *mtd,
> + const struct nand_data_interface *conf)
> +{
> + struct nand_chip *chip = mtd->priv;
> + int ret;
> +
> + if (!chip->setup_data_interface)
> + return -ENOTSUPP;
> +
> + ret = chip->setup_data_interface(mtd, conf, false);
> + if (ret)
> + return ret;
> +
> + *chip->data_iface = *conf;
> +
> + return 0;
> +}
> +
> +/**
> + * nand_setup_data_interface - Check if a data interface config is supported
s/setup/check
> + * by the NAND controller
> + * @mtd: MTD device structure
> + * @conf: new configuration to apply
> + *
> + * Check if the provided data interface configuration is supported by the
> + * NAND controller.
> + *
> + * Returns 0 if it is supported or -ERROR_CODE.
> + */
> +static int nand_check_data_interface(struct mtd_info *mtd,
> + const struct nand_data_interface *conf)
> +{
> + struct nand_chip *chip = mtd->priv;
> +
> + if (!chip->setup_data_interface)
> + return -ENOTSUPP;
> +
> + return chip->setup_data_interface(mtd, conf, true);
> +}
> +
> +/**
> + * nand_configure_data_interface - Configure the data interface and timings
> + * @mtd: MTD device structure
> + *
> + * Try to configure the data interface and NAND timings appropriately.
> + * First tries to retrieve supported timing modes from ONFI information,
> + * and if the NAND chip does not support ONFI, relies on the
> + * ->onfi_timing_mode_default specified in the nand_ids table.
> + *
> + * Returns 0 in case of success or -ERROR_CODE.
> + */
> +static int nand_configure_data_interface(struct mtd_info *mtd)
> +{
> + struct nand_chip *chip = mtd->priv;
> + struct nand_data_interface *conf;
> + int modes, mode, ret = -EINVAL;
> +
> + conf = kzalloc(sizeof(*conf), GFP_KERNEL);
> + if (!conf)
> + return -ENOMEM;
> +
> + /* TODO: support DDR interfaces */
> + conf->type = NAND_SDR_IFACE;
> +
> + /*
> + * First try to identify the best timings from ONFI parameters and
> + * if the NAND does not support ONFI, fallback to the default ONFI
> + * timing mode.
> + */
> + modes = onfi_get_async_timing_mode(chip);
> + if (modes != ONFI_TIMING_MODE_UNKNOWN) {
> + for (mode = fls(modes) - 1; mode >= 0; mode--) {
> + conf->timings.sdr =
> + *onfi_async_timing_mode_to_sdr_timings(mode);
> +
> + ret = nand_check_data_interface(mtd, conf);
> + if (!ret)
> + break;
> + }
> + } else {
> + mode = chip->onfi_timing_mode_default;
> + conf->timings.sdr =
> + *onfi_async_timing_mode_to_sdr_timings(mode);
> +
> + ret = nand_check_data_interface(mtd, conf);
> + }
> +
> + if (!ret) {
> + uint8_t tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = { mode };
> + int i;
> +
> + /*
> + * Ensure the timing mode has be changed on the chip side
> + * before changing timings on the controller side.
> + */
> + if (modes != ONFI_TIMING_MODE_UNKNOWN) {
> + /*
> + * FIXME: should we really set the timing mode on all
> + * dies?
> + */
> + for (i = 0; i < chip->numchips; i++) {
> + chip->select_chip(mtd, i);
> + ret = chip->onfi_set_features(mtd, chip,
> + ONFI_FEATURE_ADDR_TIMING_MODE,
> + tmode_param);
> + }
> + chip->select_chip(mtd, -1);
> + }
> +
> + if (!ret) {
> + ret = nand_setup_data_interface(mtd, conf);
> +
> + /*
> + * Reset the NAND chip if the data interface setup
> + * failed so that the chip goes back to a known state
> + * (timing mode 0).
> + */
> + if (ret)
> + chip->cmdfunc(mtd, NAND_CMD_RESET, -1, -1);
> + }
> + }
> +
> + kfree(conf);
> +
> + return ret;
> +}
> +
> /*
> * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
> */
> @@ -3906,6 +4044,11 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
> chip->options &= ~NAND_SAMSUNG_LP_OPTIONS;
> ident_done:
>
> + /*
> + * Setup the NAND interface (interface type + timings).
> + */
> + nand_configure_data_interface(mtd);
> +
Need to check the returned value.
Also, it doesn't feel right to configure the timings in nand_get_flash_type.
How about something like this:
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 947e74d24ee8..5fae81101c32 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -4009,11 +4009,6 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
chip->options &= ~NAND_SAMSUNG_LP_OPTIONS;
ident_done:
- /*
- * Setup the NAND interface (interface type + timings).
- */
- nand_configure_data_interface(mtd);
-
/* Try to identify manufacturer */
for (maf_idx = 0; nand_manuf_ids[maf_idx].id != 0x0; maf_idx++) {
if (nand_manuf_ids[maf_idx].id == *maf_id)
@@ -4189,6 +4184,13 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
goto err;
}
+ /*
+ * Setup the NAND interface (interface type + timings).
+ */
+ ret = nand_configure_data_interface(mtd);
+ if (ret)
+ return ret;
+
chip->select_chip(mtd, -1);
[..]
> /* Try to identify manufacturer */
> for (maf_idx = 0; nand_manuf_ids[maf_idx].id != 0x0; maf_idx++) {
> if (nand_manuf_ids[maf_idx].id == *maf_id)
> @@ -4033,6 +4176,41 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
> /* Set the default functions */
> nand_set_defaults(chip, chip->options & NAND_BUSWIDTH_16);
>
> + /*
> + * Allocate an interface config struct if the controller implements the
> + * ->apply_interface_conf() method.
> + */
> + if (chip->setup_data_interface) {
> + chip->data_iface = kzalloc(sizeof(*chip->data_iface),
> + GFP_KERNEL);
> + if (!chip->data_iface)
> + return -ENOMEM;
> +
> + /*
> + * The ONFI specification says:
> + * "
> + * To transition from NV-DDR or NV-DDR2 to the SDR data
> + * interface, the host shall use the Reset (FFh) command
> + * using SDR timing mode 0. A device in any timing mode is
> + * required to recognize Reset (FFh) command issued in SDR
> + * timing mode 0.
> + * "
> + *
> + * Configure the data interface in SDR mode and set the
> + * timings to timing mode 0. The Reset command is issued
> + * in nand_get_flash_type().
> + */
> +
> + chip->data_iface->type = NAND_SDR_IFACE;
> + chip->data_iface->timings.sdr =
> + *onfi_async_timing_mode_to_sdr_timings(0);
> + ret = chip->setup_data_interface(mtd, chip->data_iface, false);
> + if (ret) {
> + pr_err("Failed to configure data interface to SDR timing mode 0\n");
> + goto err;
> + }
> + }
> +
> /* Read the flash type */
> type = nand_get_flash_type(mtd, chip, &nand_maf_id,
> &nand_dev_id, table);
> @@ -4041,7 +4219,9 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
> if (!(chip->options & NAND_SCAN_SILENT_NODEV))
> pr_warn("No NAND device found\n");
> chip->select_chip(mtd, -1);
> - return PTR_ERR(type);
> + kfree(chip->data_iface);
You free data_iface here...
> + ret = PTR_ERR(type);
> + goto err;
> }
>
> chip->select_chip(mtd, -1);
> @@ -4069,6 +4249,10 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
> mtd->size = i * chip->chipsize;
>
> return 0;
> +
> +err:
> + kfree(chip->data_iface);
...and again here.
> + return ret;
> }
> EXPORT_SYMBOL(nand_scan_ident);
>
> @@ -4476,6 +4660,9 @@ void nand_release(struct mtd_info *mtd)
>
> mtd_device_unregister(mtd);
>
> + /* Free interface config struct */
> + kfree(chip->data_iface);
> +
> /* Free bad block table memory */
> kfree(chip->bbt);
> if (!(chip->options & NAND_OWN_BUFFERS))
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
[..]
> /**
> * struct nand_chip - NAND Private Flash Chip Data
> * @IO_ADDR_R: [BOARDSPECIFIC] address to read the 8 I/O lines of the
> @@ -690,6 +750,10 @@ struct nand_chip {
> int (*onfi_get_features)(struct mtd_info *mtd, struct nand_chip *chip,
> int feature_addr, uint8_t *subfeature_para);
> int (*setup_read_retry)(struct mtd_info *mtd, int retry_mode);
> + int (*setup_data_interface)(struct mtd_info *mtd,
> + const struct nand_data_interface *conf,
> + bool check_only);
The function lacks its proper documentation in the comment above
the struct.
--
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] mtd: nand: automate NAND timings selection
2015-11-02 0:37 ` Ezequiel Garcia
@ 2015-11-02 8:39 ` Boris Brezillon
0 siblings, 0 replies; 5+ messages in thread
From: Boris Brezillon @ 2015-11-02 8:39 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: David Woodhouse, Brian Norris, linux-mtd, Maxime Ripard,
linux-sunxi, linux-kernel
Hi Ezequiel,
On Sun, 1 Nov 2015 21:37:33 -0300
Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
> Hi Boris,
>
> This looks nice. I gave a try at this patch and it allows to simplify
> pxa3xx-nand greatly. A few comments below.
>
> On 23 Oct 01:03 PM, Boris Brezillon wrote:
> > The NAND framework provides several helpers to query timing modes supported
> > by a NAND chip, but this implies that all NAND controller drivers have
> > to implement the same timings selection dance.
> >
> > Provide a common logic to select the best timings based on ONFI or
> > ->onfi_timing_mode_default information.
> > NAND controller willing to support timings adjustment should just
> > implement the ->setup_data_interface() method.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> > drivers/mtd/nand/nand_base.c | 189 ++++++++++++++++++++++++++++++++++++++++++-
> > include/linux/mtd/nand.h | 115 +++++++++++++++-----------
> > 2 files changed, 254 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index eaf1fcd..52a1f89 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -3323,6 +3323,144 @@ static void nand_onfi_detect_micron(struct nand_chip *chip,
> > chip->setup_read_retry = nand_setup_read_retry_micron;
> > }
> >
> > +/**
> > + * nand_setup_data_interface - Setup the data interface and timings on the
> > + * controller side
> > + * @mtd: MTD device structure
> > + * @conf: new configuration to apply
> > + *
> > + * Try to configure the NAND controller to support the provided data
> > + * interface configuration.
> > + *
> > + * Returns 0 in case of success or -ERROR_CODE.
> > + */
> > +static int nand_setup_data_interface(struct mtd_info *mtd,
> > + const struct nand_data_interface *conf)
> > +{
> > + struct nand_chip *chip = mtd->priv;
> > + int ret;
> > +
> > + if (!chip->setup_data_interface)
> > + return -ENOTSUPP;
> > +
> > + ret = chip->setup_data_interface(mtd, conf, false);
> > + if (ret)
> > + return ret;
> > +
> > + *chip->data_iface = *conf;
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * nand_setup_data_interface - Check if a data interface config is supported
>
> s/setup/check
Yes, I'll fix it.
>
> > + * by the NAND controller
> > + * @mtd: MTD device structure
> > + * @conf: new configuration to apply
> > + *
> > + * Check if the provided data interface configuration is supported by the
> > + * NAND controller.
> > + *
> > + * Returns 0 if it is supported or -ERROR_CODE.
> > + */
> > +static int nand_check_data_interface(struct mtd_info *mtd,
> > + const struct nand_data_interface *conf)
> > +{
> > + struct nand_chip *chip = mtd->priv;
> > +
> > + if (!chip->setup_data_interface)
> > + return -ENOTSUPP;
> > +
> > + return chip->setup_data_interface(mtd, conf, true);
> > +}
> > +
> > +/**
> > + * nand_configure_data_interface - Configure the data interface and timings
> > + * @mtd: MTD device structure
> > + *
> > + * Try to configure the data interface and NAND timings appropriately.
> > + * First tries to retrieve supported timing modes from ONFI information,
> > + * and if the NAND chip does not support ONFI, relies on the
> > + * ->onfi_timing_mode_default specified in the nand_ids table.
> > + *
> > + * Returns 0 in case of success or -ERROR_CODE.
> > + */
> > +static int nand_configure_data_interface(struct mtd_info *mtd)
> > +{
> > + struct nand_chip *chip = mtd->priv;
> > + struct nand_data_interface *conf;
> > + int modes, mode, ret = -EINVAL;
> > +
> > + conf = kzalloc(sizeof(*conf), GFP_KERNEL);
> > + if (!conf)
> > + return -ENOMEM;
> > +
> > + /* TODO: support DDR interfaces */
> > + conf->type = NAND_SDR_IFACE;
> > +
> > + /*
> > + * First try to identify the best timings from ONFI parameters and
> > + * if the NAND does not support ONFI, fallback to the default ONFI
> > + * timing mode.
> > + */
> > + modes = onfi_get_async_timing_mode(chip);
> > + if (modes != ONFI_TIMING_MODE_UNKNOWN) {
> > + for (mode = fls(modes) - 1; mode >= 0; mode--) {
> > + conf->timings.sdr =
> > + *onfi_async_timing_mode_to_sdr_timings(mode);
> > +
> > + ret = nand_check_data_interface(mtd, conf);
> > + if (!ret)
> > + break;
> > + }
> > + } else {
> > + mode = chip->onfi_timing_mode_default;
> > + conf->timings.sdr =
> > + *onfi_async_timing_mode_to_sdr_timings(mode);
> > +
> > + ret = nand_check_data_interface(mtd, conf);
> > + }
> > +
> > + if (!ret) {
> > + uint8_t tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = { mode };
> > + int i;
> > +
> > + /*
> > + * Ensure the timing mode has be changed on the chip side
> > + * before changing timings on the controller side.
> > + */
> > + if (modes != ONFI_TIMING_MODE_UNKNOWN) {
> > + /*
> > + * FIXME: should we really set the timing mode on all
> > + * dies?
> > + */
> > + for (i = 0; i < chip->numchips; i++) {
> > + chip->select_chip(mtd, i);
> > + ret = chip->onfi_set_features(mtd, chip,
> > + ONFI_FEATURE_ADDR_TIMING_MODE,
> > + tmode_param);
> > + }
> > + chip->select_chip(mtd, -1);
> > + }
> > +
> > + if (!ret) {
> > + ret = nand_setup_data_interface(mtd, conf);
> > +
> > + /*
> > + * Reset the NAND chip if the data interface setup
> > + * failed so that the chip goes back to a known state
> > + * (timing mode 0).
> > + */
> > + if (ret)
> > + chip->cmdfunc(mtd, NAND_CMD_RESET, -1, -1);
> > + }
> > + }
> > +
> > + kfree(conf);
> > +
> > + return ret;
> > +}
> > +
> > /*
> > * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
> > */
> > @@ -3906,6 +4044,11 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
> > chip->options &= ~NAND_SAMSUNG_LP_OPTIONS;
> > ident_done:
> >
> > + /*
> > + * Setup the NAND interface (interface type + timings).
> > + */
> > + nand_configure_data_interface(mtd);
> > +
>
> Need to check the returned value.
The reason I don't check the return value here is because the
->setup_data_interface() function is not mandatory, and
nand_configure_data_interface() will return -ENOTSUPP if it's not
implemented. But maybe we should check the return value and avoid
calling this function if chip->setup_data_interface() is not
implemented.
>
> Also, it doesn't feel right to configure the timings in nand_get_flash_type.
> How about something like this:
Yes, it's probably a better place.
>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 947e74d24ee8..5fae81101c32 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -4009,11 +4009,6 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
> chip->options &= ~NAND_SAMSUNG_LP_OPTIONS;
> ident_done:
>
> - /*
> - * Setup the NAND interface (interface type + timings).
> - */
> - nand_configure_data_interface(mtd);
> -
> /* Try to identify manufacturer */
> for (maf_idx = 0; nand_manuf_ids[maf_idx].id != 0x0; maf_idx++) {
> if (nand_manuf_ids[maf_idx].id == *maf_id)
> @@ -4189,6 +4184,13 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
> goto err;
> }
>
> + /*
> + * Setup the NAND interface (interface type + timings).
> + */
> + ret = nand_configure_data_interface(mtd);
> + if (ret)
> + return ret;
> +
> chip->select_chip(mtd, -1);
> [..]
>
>
> > /* Try to identify manufacturer */
> > for (maf_idx = 0; nand_manuf_ids[maf_idx].id != 0x0; maf_idx++) {
> > if (nand_manuf_ids[maf_idx].id == *maf_id)
> > @@ -4033,6 +4176,41 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
> > /* Set the default functions */
> > nand_set_defaults(chip, chip->options & NAND_BUSWIDTH_16);
> >
> > + /*
> > + * Allocate an interface config struct if the controller implements the
> > + * ->apply_interface_conf() method.
> > + */
> > + if (chip->setup_data_interface) {
> > + chip->data_iface = kzalloc(sizeof(*chip->data_iface),
> > + GFP_KERNEL);
> > + if (!chip->data_iface)
> > + return -ENOMEM;
> > +
> > + /*
> > + * The ONFI specification says:
> > + * "
> > + * To transition from NV-DDR or NV-DDR2 to the SDR data
> > + * interface, the host shall use the Reset (FFh) command
> > + * using SDR timing mode 0. A device in any timing mode is
> > + * required to recognize Reset (FFh) command issued in SDR
> > + * timing mode 0.
> > + * "
> > + *
> > + * Configure the data interface in SDR mode and set the
> > + * timings to timing mode 0. The Reset command is issued
> > + * in nand_get_flash_type().
> > + */
> > +
> > + chip->data_iface->type = NAND_SDR_IFACE;
> > + chip->data_iface->timings.sdr =
> > + *onfi_async_timing_mode_to_sdr_timings(0);
> > + ret = chip->setup_data_interface(mtd, chip->data_iface, false);
> > + if (ret) {
> > + pr_err("Failed to configure data interface to SDR timing mode 0\n");
> > + goto err;
> > + }
> > + }
> > +
> > /* Read the flash type */
> > type = nand_get_flash_type(mtd, chip, &nand_maf_id,
> > &nand_dev_id, table);
> > @@ -4041,7 +4219,9 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
> > if (!(chip->options & NAND_SCAN_SILENT_NODEV))
> > pr_warn("No NAND device found\n");
> > chip->select_chip(mtd, -1);
> > - return PTR_ERR(type);
> > + kfree(chip->data_iface);
>
> You free data_iface here...
Oops, probably a leftover of a previous version where I was not freeing
chip->data_iface in the error path.
I'll fix that.
>
> > + ret = PTR_ERR(type);
> > + goto err;
> > }
> >
> > chip->select_chip(mtd, -1);
> > @@ -4069,6 +4249,10 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
> > mtd->size = i * chip->chipsize;
> >
> > return 0;
> > +
> > +err:
> > + kfree(chip->data_iface);
>
> ...and again here.
>
> > + return ret;
> > }
> > EXPORT_SYMBOL(nand_scan_ident);
> >
> > @@ -4476,6 +4660,9 @@ void nand_release(struct mtd_info *mtd)
> >
> > mtd_device_unregister(mtd);
> >
> > + /* Free interface config struct */
> > + kfree(chip->data_iface);
> > +
> > /* Free bad block table memory */
> > kfree(chip->bbt);
> > if (!(chip->options & NAND_OWN_BUFFERS))
> > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> [..]
> > /**
> > * struct nand_chip - NAND Private Flash Chip Data
> > * @IO_ADDR_R: [BOARDSPECIFIC] address to read the 8 I/O lines of the
> > @@ -690,6 +750,10 @@ struct nand_chip {
> > int (*onfi_get_features)(struct mtd_info *mtd, struct nand_chip *chip,
> > int feature_addr, uint8_t *subfeature_para);
> > int (*setup_read_retry)(struct mtd_info *mtd, int retry_mode);
> > + int (*setup_data_interface)(struct mtd_info *mtd,
> > + const struct nand_data_interface *conf,
> > + bool check_only);
>
> The function lacks its proper documentation in the comment above
> the struct.
>
Yep, I'll document this new function.
Thanks for your review.
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-11-02 8:39 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-23 11:03 [PATCH 0/2] mtd: nand: automate NAND timings selection Boris Brezillon
2015-10-23 11:03 ` [PATCH 1/2] " Boris Brezillon
2015-11-02 0:37 ` Ezequiel Garcia
2015-11-02 8:39 ` Boris Brezillon
2015-10-23 11:03 ` [PATCH 2/2] mtd: nand: sunxi: switch from manual to automated timing config Boris Brezillon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®