[PATCH 0/2] mtd: nand: Enforce tCCS wait time after a column change

[PATCH 0/2] mtd: nand: Enforce tCCS wait time after a column change

[Boris Brezillon]

Hi,

Marc recently struggled with the RNDIN/RNDOUT commands when trying to
add support for the Tango NAND controller.
After some investigation it appeared that nothing in the
nand_command_lp() code waits tCCS, which is required to make sure the
NAND chip is ready to receive/send data on the bus after a column
change.

This series adds some more timings to the nand_sdr_timings struct
(including tCCS) and try to extract them from the ONFI parameter table
(if available).

It then adds a new flags to ask the core to enforce the tCCS
constraint.
As noted in the commit message, this is an opt-in flag to avoid perf
regressions on existing implementations (adding an ndelay() might be
useless if the controller IP or driver already takes care of that),
but it might appear that some implementations are actually broken and
needs this flag as well.

Regards,

Boris

Boris Brezillon (2):
  mtd: nand: Add a few more timings to nand_sdr_timings
  mtd: nand: Wait tCCS after a column change

 drivers/mtd/nand/nand_base.c    | 26 +++++++++++++++++++++++++-
 drivers/mtd/nand/nand_timings.c | 26 +++++++++++++++++++++++++-
 include/linux/mtd/nand.h        | 18 ++++++++++++++++++
 3 files changed, 68 insertions(+), 2 deletions(-)

-- 
2.7.4

[PATCH 1/2] mtd: nand: Add a few more timings to nand_sdr_timings

[Boris Brezillon]

Add the tR_max, tBERS_max, tPROG_max and tCCS_min timings to the
nand_sdr_timings struct.
Assign default/safe values for the statically defined timings, and
extract them from the ONFI parameter table if the NAND is ONFI
compliant.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/nand/nand_timings.c | 26 +++++++++++++++++++++++++-
 include/linux/mtd/nand.h        |  8 ++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/nand_timings.c b/drivers/mtd/nand/nand_timings.c
index 13a587407be3..f06312df3669 100644
--- a/drivers/mtd/nand/nand_timings.c
+++ b/drivers/mtd/nand/nand_timings.c
@@ -18,6 +18,8 @@ static const struct nand_data_interface onfi_sdr_timings[] = {
 	{
 		.type = NAND_SDR_IFACE,
 		.timings.sdr = {
+			.tCCS_min = 500000,
+			.tR_max = 200000000,
 			.tADL_min = 400000,
 			.tALH_min = 20000,
 			.tALS_min = 50000,
@@ -58,6 +60,8 @@ static const struct nand_data_interface onfi_sdr_timings[] = {
 	{
 		.type = NAND_SDR_IFACE,
 		.timings.sdr = {
+			.tCCS_min = 500000,
+			.tR_max = 200000000,
 			.tADL_min = 400000,
 			.tALH_min = 10000,
 			.tALS_min = 25000,
@@ -98,6 +102,8 @@ static const struct nand_data_interface onfi_sdr_timings[] = {
 	{
 		.type = NAND_SDR_IFACE,
 		.timings.sdr = {
+			.tCCS_min = 500000,
+			.tR_max = 200000000,
 			.tADL_min = 400000,
 			.tALH_min = 10000,
 			.tALS_min = 15000,
@@ -138,6 +144,8 @@ static const struct nand_data_interface onfi_sdr_timings[] = {
 	{
 		.type = NAND_SDR_IFACE,
 		.timings.sdr = {
+			.tCCS_min = 500000,
+			.tR_max = 200000000,
 			.tADL_min = 400000,
 			.tALH_min = 5000,
 			.tALS_min = 10000,
@@ -178,6 +186,8 @@ static const struct nand_data_interface onfi_sdr_timings[] = {
 	{
 		.type = NAND_SDR_IFACE,
 		.timings.sdr = {
+			.tCCS_min = 500000,
+			.tR_max = 200000000,
 			.tADL_min = 400000,
 			.tALH_min = 5000,
 			.tALS_min = 10000,
@@ -218,6 +228,8 @@ static const struct nand_data_interface onfi_sdr_timings[] = {
 	{
 		.type = NAND_SDR_IFACE,
 		.timings.sdr = {
+			.tCCS_min = 500000,
+			.tR_max = 200000000,
 			.tADL_min = 400000,
 			.tALH_min = 5000,
 			.tALS_min = 10000,
@@ -290,10 +302,22 @@ int onfi_init_data_interface(struct nand_chip *chip,
 	*iface = onfi_sdr_timings[timing_mode];
 
 	/*
-	 * TODO: initialize timings that cannot be deduced from timing mode:
+	 * Initialize timings that cannot be deduced from timing mode:
 	 * tR, tPROG, tCCS, ...
 	 * These information are part of the ONFI parameter page.
 	 */
+	if (chip->onfi_version) {
+		struct nand_onfi_params *params = &chip->onfi_params;
+		struct nand_sdr_timings *timings = &iface->timings.sdr;
+
+		/* microseconds -> picoseconds */
+		timings->tPROG_max = 1000000UL * le16_to_cpu(params->t_prog);
+		timings->tBERS_max = 1000000UL * le16_to_cpu(params->t_bers);
+		timings->tR_max = 1000000UL * le16_to_cpu(params->t_r);
+
+		/* nanoseconds -> picoseconds */
+		timings->tCCS_min = 1000UL * le16_to_cpu(params->t_ccs);
+	}
 
 	return 0;
 }
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index c5d3d5024fc8..6fe83bce83a6 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -584,6 +584,10 @@ struct nand_buffers {
  *
  * All these timings are expressed in picoseconds.
  *
+ * @tBERS_max: Block erase time
+ * @tCCS_min: Change column setup time
+ * @tPROG_max: Page program time
+ * @tR_max: Page read time
  * @tALH_min: ALE hold time
  * @tADL_min: ALE to data loading time
  * @tALS_min: ALE setup time
@@ -621,6 +625,10 @@ struct nand_buffers {
  * @tWW_min: WP# transition to WE# low
  */
 struct nand_sdr_timings {
+	u32 tBERS_max;
+	u32 tCCS_min;
+	u32 tPROG_max;
+	u32 tR_max;
 	u32 tALH_min;
 	u32 tADL_min;
 	u32 tALS_min;
-- 
2.7.4

[PATCH 2/2] mtd: nand: Wait tCCS after a column change

[Boris Brezillon]

Drivers implementing ->cmd_ctrl() and relying on the default ->cmdfunc()
implementation usually don't wait tCCS when a column change (RNDIN or
RNDOUT) is requested.
Add an option flag to ask the core to do so (note that we keep this as
an opt-in to avoid breaking existing implementations), and make use of
the ->data_interface information is available (otherwise, wait 500ns).

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/nand/nand_base.c | 26 +++++++++++++++++++++++++-
 include/linux/mtd/nand.h     | 10 ++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 7a00245ebada..3b0e6d861e2e 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -709,6 +709,25 @@ static void nand_command(struct mtd_info *mtd, unsigned int command,
 	nand_wait_ready(mtd);
 }
 
+static void nand_ccs_delay(struct nand_chip *chip)
+{
+	/*
+	 * The controller already takes care of waiting for tCCS when the RNDIN
+	 * or RNDOUT command is sent, return directly.
+	 */
+	if (!(chip->options & NAND_WAIT_TCCS))
+		return;
+
+	/*
+	 * Wait tCCS_min if it is correctly defined, otherwise wait 500ns
+	 * (which should be safe for all NANDs).
+	 */
+	if (chip->data_interface && chip->data_interface->timings.sdr.tCCS_min)
+		ndelay(chip->data_interface->timings.sdr.tCCS_min / 1000);
+	else
+		ndelay(500);
+}
+
 /**
  * nand_command_lp - [DEFAULT] Send command to NAND large page device
  * @mtd: MTD device structure
@@ -773,10 +792,13 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command,
 	case NAND_CMD_ERASE1:
 	case NAND_CMD_ERASE2:
 	case NAND_CMD_SEQIN:
-	case NAND_CMD_RNDIN:
 	case NAND_CMD_STATUS:
 		return;
 
+	case NAND_CMD_RNDIN:
+		nand_ccs_delay(chip);
+		return;
+
 	case NAND_CMD_RESET:
 		if (chip->dev_ready)
 			break;
@@ -795,6 +817,8 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command,
 			       NAND_NCE | NAND_CLE | NAND_CTRL_CHANGE);
 		chip->cmd_ctrl(mtd, NAND_CMD_NONE,
 			       NAND_NCE | NAND_CTRL_CHANGE);
+
+		nand_ccs_delay(chip);
 		return;
 
 	case NAND_CMD_READ0:
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 6fe83bce83a6..970ceb948835 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -210,6 +210,16 @@ enum nand_ecc_algo {
  */
 #define NAND_USE_BOUNCE_BUFFER	0x00100000
 
+/*
+ * In case your controller is implementing ->cmd_ctrl() and is relying on the
+ * default ->cmdfunc() implementation, you may want to let the core handle the
+ * tCCS delay which is required when a column change (RNDIN or RNDOUT) is
+ * requested.
+ * If your controller already takes care of this delay, you don't need to set
+ * this flag.
+ */
+#define NAND_WAIT_TCCS		0x00200000
+
 /* Options set by nand scan */
 /* Nand scan has allocated controller struct */
 #define NAND_CONTROLLER_ALLOC	0x80000000
-- 
2.7.4

Re: [PATCH 0/2] mtd: nand: Enforce tCCS wait time after a column change

[Marc Gonzalez]

On 01/10/2016 10:24, Boris Brezillon wrote:

> Marc recently struggled with the RNDIN/RNDOUT commands when trying to
> add support for the Tango NAND controller.
> After some investigation it appeared that nothing in the
> nand_command_lp() code waits tCCS, which is required to make sure the
> NAND chip is ready to receive/send data on the bus after a column
> change.
> 
> This series adds some more timings to the nand_sdr_timings struct
> (including tCCS) and try to extract them from the ONFI parameter table
> (if available).
> 
> It then adds a new flags to ask the core to enforce the tCCS
> constraint.
> As noted in the commit message, this is an opt-in flag to avoid perf
> regressions on existing implementations (adding an ndelay() might be
> useless if the controller IP or driver already takes care of that),
> but it might appear that some implementations are actually broken and
> needs this flag as well.
> 
> Regards,
> 
> Boris
> 
> Boris Brezillon (2):
>   mtd: nand: Add a few more timings to nand_sdr_timings
>   mtd: nand: Wait tCCS after a column change
> 
>  drivers/mtd/nand/nand_base.c    | 26 +++++++++++++++++++++++++-
>  drivers/mtd/nand/nand_timings.c | 26 +++++++++++++++++++++++++-
>  include/linux/mtd/nand.h        | 18 ++++++++++++++++++
>  3 files changed, 68 insertions(+), 2 deletions(-)

Tested-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>

Regards.

Re: [PATCH 0/2] mtd: nand: Enforce tCCS wait time after a column change

[Boris Brezillon]

On Sat,  1 Oct 2016 10:24:01 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> Hi,
> 
> Marc recently struggled with the RNDIN/RNDOUT commands when trying to
> add support for the Tango NAND controller.
> After some investigation it appeared that nothing in the
> nand_command_lp() code waits tCCS, which is required to make sure the
> NAND chip is ready to receive/send data on the bus after a column
> change.
> 
> This series adds some more timings to the nand_sdr_timings struct
> (including tCCS) and try to extract them from the ONFI parameter table
> (if available).
> 
> It then adds a new flags to ask the core to enforce the tCCS
> constraint.
> As noted in the commit message, this is an opt-in flag to avoid perf
> regressions on existing implementations (adding an ndelay() might be
> useless if the controller IP or driver already takes care of that),
> but it might appear that some implementations are actually broken and
> needs this flag as well.

Applied.

> 
> Regards,
> 
> Boris
> 
> Boris Brezillon (2):
>   mtd: nand: Add a few more timings to nand_sdr_timings
>   mtd: nand: Wait tCCS after a column change
> 
>  drivers/mtd/nand/nand_base.c    | 26 +++++++++++++++++++++++++-
>  drivers/mtd/nand/nand_timings.c | 26 +++++++++++++++++++++++++-
>  include/linux/mtd/nand.h        | 18 ++++++++++++++++++
>  3 files changed, 68 insertions(+), 2 deletions(-)
>