* [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 related [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 related [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 related [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.