[PATCH v2 0/4] mtd: rawnand: Fixes nand infra delay setting and FSMC nand controller

[PATCH v2 0/4] mtd: rawnand: Fixes nand infra delay setting and FSMC nand controller

[Herve Codina]

Hi,

This patch series is mainly related to FSMC nand controller fixes except
for the two first patches which fix some issues in nand base.

Details are given in each patches.

Related to the v1 series:
- the patch related to 8bit accesses has been removed as it was
  not needed (I misunderstood 8bit accesses meaning).
- A new patch is introduced related to nand_choose_best_timings()
  issue I discovered during this v2 testing.

Best regards,
Herve Codina

Herve Codina (4):
  mtd: rawnand: Fix nand_erase_op delay
  mtd: rawnand: Fix nand_choose_best_timings() on unsupported interface
  mtd: rawnand: fsmc: Take instruction delay into account
  mtd: rawnand: fsmc: Fix timing computation

 drivers/mtd/nand/raw/fsmc_nand.c | 36 +++++++++++++++++++++++++-------
 drivers/mtd/nand/raw/nand_base.c |  6 +++---
 2 files changed, 31 insertions(+), 11 deletions(-)

-- 
2.31.1

[PATCH v2 0/4] mtd: rawnand: Fixes nand infra delay setting and FSMC nand controller

[Herve Codina]

Hi,

This patch series is mainly related to FSMC nand controller fixes except
for the two first patches which fix some issues in nand base.

Details are given in each patches.

Related to the v1 series:
- the patch related to 8bit accesses has been removed as it was
  not needed (I misunderstood 8bit accesses meaning).
- A new patch is introduced related to nand_choose_best_timings()
  issue I discovered during this v2 testing.

Best regards,
Herve Codina

Herve Codina (4):
  mtd: rawnand: Fix nand_erase_op delay
  mtd: rawnand: Fix nand_choose_best_timings() on unsupported interface
  mtd: rawnand: fsmc: Take instruction delay into account
  mtd: rawnand: fsmc: Fix timing computation

 drivers/mtd/nand/raw/fsmc_nand.c | 36 +++++++++++++++++++++++++-------
 drivers/mtd/nand/raw/nand_base.c |  6 +++---
 2 files changed, 31 insertions(+), 11 deletions(-)

-- 
2.31.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 1/4] mtd: rawnand: Fix nand_erase_op delay

[Herve Codina]

NAND_OP_CMD() expect a delay parameter in nanoseconds.
The delay value is wrongly given in milliseconds.

Fix the conversion macro used in order to set this
delay in nanoseconds.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
Changes v1 -> v2:
- Commit log reword

 drivers/mtd/nand/raw/nand_base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 3d6c6e880520..5c6b065837ef 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -1837,7 +1837,7 @@ int nand_erase_op(struct nand_chip *chip, unsigned int eraseblock)
 			NAND_OP_CMD(NAND_CMD_ERASE1, 0),
 			NAND_OP_ADDR(2, addrs, 0),
 			NAND_OP_CMD(NAND_CMD_ERASE2,
-				    NAND_COMMON_TIMING_MS(conf, tWB_max)),
+				    NAND_COMMON_TIMING_NS(conf, tWB_max)),
 			NAND_OP_WAIT_RDY(NAND_COMMON_TIMING_MS(conf, tBERS_max),
 					 0),
 		};
-- 
2.31.1

[PATCH v2 1/4] mtd: rawnand: Fix nand_erase_op delay

[Herve Codina]

NAND_OP_CMD() expect a delay parameter in nanoseconds.
The delay value is wrongly given in milliseconds.

Fix the conversion macro used in order to set this
delay in nanoseconds.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
Changes v1 -> v2:
- Commit log reword

 drivers/mtd/nand/raw/nand_base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 3d6c6e880520..5c6b065837ef 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -1837,7 +1837,7 @@ int nand_erase_op(struct nand_chip *chip, unsigned int eraseblock)
 			NAND_OP_CMD(NAND_CMD_ERASE1, 0),
 			NAND_OP_ADDR(2, addrs, 0),
 			NAND_OP_CMD(NAND_CMD_ERASE2,
-				    NAND_COMMON_TIMING_MS(conf, tWB_max)),
+				    NAND_COMMON_TIMING_NS(conf, tWB_max)),
 			NAND_OP_WAIT_RDY(NAND_COMMON_TIMING_MS(conf, tBERS_max),
 					 0),
 		};
-- 
2.31.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 2/4] mtd: rawnand: Fix nand_choose_best_timings() on unsupported interface

[Herve Codina]

When the NV-DDR interface is not supported by the NAND chip,
the value of onfi->nvddr_timing_modes is 0. In this case,
the best_mode variable value in nand_choose_best_nvddr_timings()
is -1. The last for-loop is skipped and the function returns an
uninitialized value.
If this returned value is 0, the nand_choose_best_sdr_timings()
is not executed and no 'best timing' are set. This leads the host
controller and the NAND chip working at default mode 0 timing
even if a better timing can be used.

Fix this uninitialzed returned value.

nand_choose_best_sdr_timings() is pretty similar to
nand_choose_best_nvddr_timings(). Even if onfi->sdr_timing_modes
should never be seen as 0, nand_choose_best_sdr_timings() returned
value is fixed.

Fixes: a9ecc8c814e9 ("mtd: rawnand: Choose the best timings, NV-DDR included")
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
Changes v1 to v2:
- New patch in v2 series

 drivers/mtd/nand/raw/nand_base.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 5c6b065837ef..a130320de412 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -926,7 +926,7 @@ int nand_choose_best_sdr_timings(struct nand_chip *chip,
 				 struct nand_sdr_timings *spec_timings)
 {
 	const struct nand_controller_ops *ops = chip->controller->ops;
-	int best_mode = 0, mode, ret;
+	int best_mode = 0, mode, ret = -EOPNOTSUPP;
 
 	iface->type = NAND_SDR_IFACE;
 
@@ -977,7 +977,7 @@ int nand_choose_best_nvddr_timings(struct nand_chip *chip,
 				   struct nand_nvddr_timings *spec_timings)
 {
 	const struct nand_controller_ops *ops = chip->controller->ops;
-	int best_mode = 0, mode, ret;
+	int best_mode = 0, mode, ret = -EOPNOTSUPP;
 
 	iface->type = NAND_NVDDR_IFACE;
 
-- 
2.31.1

[PATCH v2 2/4] mtd: rawnand: Fix nand_choose_best_timings() on unsupported interface

[Herve Codina]

When the NV-DDR interface is not supported by the NAND chip,
the value of onfi->nvddr_timing_modes is 0. In this case,
the best_mode variable value in nand_choose_best_nvddr_timings()
is -1. The last for-loop is skipped and the function returns an
uninitialized value.
If this returned value is 0, the nand_choose_best_sdr_timings()
is not executed and no 'best timing' are set. This leads the host
controller and the NAND chip working at default mode 0 timing
even if a better timing can be used.

Fix this uninitialzed returned value.

nand_choose_best_sdr_timings() is pretty similar to
nand_choose_best_nvddr_timings(). Even if onfi->sdr_timing_modes
should never be seen as 0, nand_choose_best_sdr_timings() returned
value is fixed.

Fixes: a9ecc8c814e9 ("mtd: rawnand: Choose the best timings, NV-DDR included")
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
Changes v1 to v2:
- New patch in v2 series

 drivers/mtd/nand/raw/nand_base.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 5c6b065837ef..a130320de412 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -926,7 +926,7 @@ int nand_choose_best_sdr_timings(struct nand_chip *chip,
 				 struct nand_sdr_timings *spec_timings)
 {
 	const struct nand_controller_ops *ops = chip->controller->ops;
-	int best_mode = 0, mode, ret;
+	int best_mode = 0, mode, ret = -EOPNOTSUPP;
 
 	iface->type = NAND_SDR_IFACE;
 
@@ -977,7 +977,7 @@ int nand_choose_best_nvddr_timings(struct nand_chip *chip,
 				   struct nand_nvddr_timings *spec_timings)
 {
 	const struct nand_controller_ops *ops = chip->controller->ops;
-	int best_mode = 0, mode, ret;
+	int best_mode = 0, mode, ret = -EOPNOTSUPP;
 
 	iface->type = NAND_NVDDR_IFACE;
 
-- 
2.31.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 3/4] mtd: rawnand: fsmc: Take instruction delay into account

[Herve Codina]

The FSMC NAND controller should apply a delay after the
instruction has been issued on the bus.
The FSMC NAND controller driver did not handle this delay.

Add this waiting delay in the FSMC NAND controller driver.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
Changes v1 -> v2:
- Commit log reword

 drivers/mtd/nand/raw/fsmc_nand.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mtd/nand/raw/fsmc_nand.c b/drivers/mtd/nand/raw/fsmc_nand.c
index 658f0cbe7ce8..0a6c9ef0ea8b 100644
--- a/drivers/mtd/nand/raw/fsmc_nand.c
+++ b/drivers/mtd/nand/raw/fsmc_nand.c
@@ -15,6 +15,7 @@
 
 #include <linux/clk.h>
 #include <linux/completion.h>
+#include <linux/delay.h>
 #include <linux/dmaengine.h>
 #include <linux/dma-direction.h>
 #include <linux/dma-mapping.h>
@@ -664,6 +665,9 @@ static int fsmc_exec_op(struct nand_chip *chip, const struct nand_operation *op,
 						instr->ctx.waitrdy.timeout_ms);
 			break;
 		}
+
+		if (instr->delay_ns)
+			ndelay(instr->delay_ns);
 	}
 
 	return ret;
-- 
2.31.1

[PATCH v2 3/4] mtd: rawnand: fsmc: Take instruction delay into account

[Herve Codina]

The FSMC NAND controller should apply a delay after the
instruction has been issued on the bus.
The FSMC NAND controller driver did not handle this delay.

Add this waiting delay in the FSMC NAND controller driver.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
Changes v1 -> v2:
- Commit log reword

 drivers/mtd/nand/raw/fsmc_nand.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mtd/nand/raw/fsmc_nand.c b/drivers/mtd/nand/raw/fsmc_nand.c
index 658f0cbe7ce8..0a6c9ef0ea8b 100644
--- a/drivers/mtd/nand/raw/fsmc_nand.c
+++ b/drivers/mtd/nand/raw/fsmc_nand.c
@@ -15,6 +15,7 @@
 
 #include <linux/clk.h>
 #include <linux/completion.h>
+#include <linux/delay.h>
 #include <linux/dmaengine.h>
 #include <linux/dma-direction.h>
 #include <linux/dma-mapping.h>
@@ -664,6 +665,9 @@ static int fsmc_exec_op(struct nand_chip *chip, const struct nand_operation *op,
 						instr->ctx.waitrdy.timeout_ms);
 			break;
 		}
+
+		if (instr->delay_ns)
+			ndelay(instr->delay_ns);
 	}
 
 	return ret;
-- 
2.31.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 4/4] mtd: rawnand: fsmc: Fix timing computation

[Herve Codina]

Under certain circumstances, the timing settings calculated by
the FSMC NAND controller driver were inaccurate.
These settings lead to incorrect data reads or fallback to
mode 0 timings depending on NAND chip used.

The timing computation did not take into account the following
constraint given in SPEAr3xx reference manual:
  twait >= tCEA - tset*TCLK + TOUTDEL + TINDEL

Enhance the timings calculation by taking into account this
additional constraint.

The change has no impact on slow timing mode such as mode 0.
Indeed, on mode 0 timing, computed values are the same with and
without the patch.

The NAND that previously used the mode 0 fallback timing is
working in mode 3 timing.
The read/writes accesses performances (flash_speed tool result)
are the following:
                        mode0       mode3
eraseblock write speed  3220 KiB/s  4511 KiB/s
eraseblock read speed   4491 KiB/s  7529 KiB/s

The NAND where data reads were previously incorrect are correct
and this NAND is also working at mode 3 timing.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
Changes v1 to v2:
- Commit log reword
- Added performance details in commit log
- Used #define for TOUTDEL and TINDEL and
  Fixed coding style
- Used max3()

 drivers/mtd/nand/raw/fsmc_nand.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/nand/raw/fsmc_nand.c b/drivers/mtd/nand/raw/fsmc_nand.c
index 0a6c9ef0ea8b..56b9da252346 100644
--- a/drivers/mtd/nand/raw/fsmc_nand.c
+++ b/drivers/mtd/nand/raw/fsmc_nand.c
@@ -94,6 +94,14 @@
 
 #define FSMC_BUSY_WAIT_TIMEOUT	(1 * HZ)
 
+/*
+ * According to SPEAr300 Reference Manual (RM0082)
+ *  TOUDEL = 7ns (Output delay from the flip-flops to the board)
+ *  TINDEL = 5ns (Input delay from the board to the flipflop)
+ */
+#define TOUTDEL	7000
+#define TINDEL	5000
+
 struct fsmc_nand_timings {
 	u8 tclr;
 	u8 tar;
@@ -278,7 +286,7 @@ static int fsmc_calc_timings(struct fsmc_nand_data *host,
 {
 	unsigned long hclk = clk_get_rate(host->clk);
 	unsigned long hclkn = NSEC_PER_SEC / hclk;
-	u32 thiz, thold, twait, tset;
+	u32 thiz, thold, twait, tset, twait_min;
 
 	if (sdrt->tRC_min < 30000)
 		return -EOPNOTSUPP;
@@ -310,13 +318,6 @@ static int fsmc_calc_timings(struct fsmc_nand_data *host,
 	else if (tims->thold > FSMC_THOLD_MASK)
 		tims->thold = FSMC_THOLD_MASK;
 
-	twait = max(sdrt->tRP_min, sdrt->tWP_min);
-	tims->twait = DIV_ROUND_UP(twait / 1000, hclkn) - 1;
-	if (tims->twait == 0)
-		tims->twait = 1;
-	else if (tims->twait > FSMC_TWAIT_MASK)
-		tims->twait = FSMC_TWAIT_MASK;
-
 	tset = max(sdrt->tCS_min - sdrt->tWP_min,
 		   sdrt->tCEA_max - sdrt->tREA_max);
 	tims->tset = DIV_ROUND_UP(tset / 1000, hclkn) - 1;
@@ -325,6 +326,21 @@ static int fsmc_calc_timings(struct fsmc_nand_data *host,
 	else if (tims->tset > FSMC_TSET_MASK)
 		tims->tset = FSMC_TSET_MASK;
 
+	/*
+	 * According to SPEAr300 Reference Manual (RM0082) which gives more
+	 * information related to FSMSC timings than the SPEAr600 one (RM0305),
+	 *   twait >= tCEA - tset*TCLK + TOUTDEL + TINDEL
+	 */
+	twait_min = sdrt->tCEA_max - ((tims->tset + 1) * hclkn * 1000)
+		    + TOUTDEL + TINDEL;
+	twait = max3(sdrt->tRP_min, sdrt->tWP_min, twait_min);
+
+	tims->twait = DIV_ROUND_UP(twait / 1000, hclkn) - 1;
+	if (tims->twait == 0)
+		tims->twait = 1;
+	else if (tims->twait > FSMC_TWAIT_MASK)
+		tims->twait = FSMC_TWAIT_MASK;
+
 	return 0;
 }
 
-- 
2.31.1

[PATCH v2 4/4] mtd: rawnand: fsmc: Fix timing computation

[Herve Codina]

Under certain circumstances, the timing settings calculated by
the FSMC NAND controller driver were inaccurate.
These settings lead to incorrect data reads or fallback to
mode 0 timings depending on NAND chip used.

The timing computation did not take into account the following
constraint given in SPEAr3xx reference manual:
  twait >= tCEA - tset*TCLK + TOUTDEL + TINDEL

Enhance the timings calculation by taking into account this
additional constraint.

The change has no impact on slow timing mode such as mode 0.
Indeed, on mode 0 timing, computed values are the same with and
without the patch.

The NAND that previously used the mode 0 fallback timing is
working in mode 3 timing.
The read/writes accesses performances (flash_speed tool result)
are the following:
                        mode0       mode3
eraseblock write speed  3220 KiB/s  4511 KiB/s
eraseblock read speed   4491 KiB/s  7529 KiB/s

The NAND where data reads were previously incorrect are correct
and this NAND is also working at mode 3 timing.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
Changes v1 to v2:
- Commit log reword
- Added performance details in commit log
- Used #define for TOUTDEL and TINDEL and
  Fixed coding style
- Used max3()

 drivers/mtd/nand/raw/fsmc_nand.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/nand/raw/fsmc_nand.c b/drivers/mtd/nand/raw/fsmc_nand.c
index 0a6c9ef0ea8b..56b9da252346 100644
--- a/drivers/mtd/nand/raw/fsmc_nand.c
+++ b/drivers/mtd/nand/raw/fsmc_nand.c
@@ -94,6 +94,14 @@
 
 #define FSMC_BUSY_WAIT_TIMEOUT	(1 * HZ)
 
+/*
+ * According to SPEAr300 Reference Manual (RM0082)
+ *  TOUDEL = 7ns (Output delay from the flip-flops to the board)
+ *  TINDEL = 5ns (Input delay from the board to the flipflop)
+ */
+#define TOUTDEL	7000
+#define TINDEL	5000
+
 struct fsmc_nand_timings {
 	u8 tclr;
 	u8 tar;
@@ -278,7 +286,7 @@ static int fsmc_calc_timings(struct fsmc_nand_data *host,
 {
 	unsigned long hclk = clk_get_rate(host->clk);
 	unsigned long hclkn = NSEC_PER_SEC / hclk;
-	u32 thiz, thold, twait, tset;
+	u32 thiz, thold, twait, tset, twait_min;
 
 	if (sdrt->tRC_min < 30000)
 		return -EOPNOTSUPP;
@@ -310,13 +318,6 @@ static int fsmc_calc_timings(struct fsmc_nand_data *host,
 	else if (tims->thold > FSMC_THOLD_MASK)
 		tims->thold = FSMC_THOLD_MASK;
 
-	twait = max(sdrt->tRP_min, sdrt->tWP_min);
-	tims->twait = DIV_ROUND_UP(twait / 1000, hclkn) - 1;
-	if (tims->twait == 0)
-		tims->twait = 1;
-	else if (tims->twait > FSMC_TWAIT_MASK)
-		tims->twait = FSMC_TWAIT_MASK;
-
 	tset = max(sdrt->tCS_min - sdrt->tWP_min,
 		   sdrt->tCEA_max - sdrt->tREA_max);
 	tims->tset = DIV_ROUND_UP(tset / 1000, hclkn) - 1;
@@ -325,6 +326,21 @@ static int fsmc_calc_timings(struct fsmc_nand_data *host,
 	else if (tims->tset > FSMC_TSET_MASK)
 		tims->tset = FSMC_TSET_MASK;
 
+	/*
+	 * According to SPEAr300 Reference Manual (RM0082) which gives more
+	 * information related to FSMSC timings than the SPEAr600 one (RM0305),
+	 *   twait >= tCEA - tset*TCLK + TOUTDEL + TINDEL
+	 */
+	twait_min = sdrt->tCEA_max - ((tims->tset + 1) * hclkn * 1000)
+		    + TOUTDEL + TINDEL;
+	twait = max3(sdrt->tRP_min, sdrt->tWP_min, twait_min);
+
+	tims->twait = DIV_ROUND_UP(twait / 1000, hclkn) - 1;
+	if (tims->twait == 0)
+		tims->twait = 1;
+	else if (tims->twait > FSMC_TWAIT_MASK)
+		tims->twait = FSMC_TWAIT_MASK;
+
 	return 0;
 }
 
-- 
2.31.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 1/4] mtd: rawnand: Fix nand_erase_op delay

[Miquel Raynal]

Hi Herve,

herve.codina@bootlin.com wrote on Fri, 19 Nov 2021 08:39:06 +0100:

> NAND_OP_CMD() expect a delay parameter in nanoseconds.

expects

> The delay value is wrongly given in milliseconds.
> 
> Fix the conversion macro used in order to set this
> delay in nanoseconds.
> 

I believe you still miss the Fixes: tag here :)

> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
> Changes v1 -> v2:
> - Commit log reword
> 
>  drivers/mtd/nand/raw/nand_base.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 3d6c6e880520..5c6b065837ef 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -1837,7 +1837,7 @@ int nand_erase_op(struct nand_chip *chip, unsigned int eraseblock)
>  			NAND_OP_CMD(NAND_CMD_ERASE1, 0),
>  			NAND_OP_ADDR(2, addrs, 0),
>  			NAND_OP_CMD(NAND_CMD_ERASE2,
> -				    NAND_COMMON_TIMING_MS(conf, tWB_max)),
> +				    NAND_COMMON_TIMING_NS(conf, tWB_max)),
>  			NAND_OP_WAIT_RDY(NAND_COMMON_TIMING_MS(conf, tBERS_max),
>  					 0),
>  		};

Thanks,
Miquèl

Re: [PATCH v2 1/4] mtd: rawnand: Fix nand_erase_op delay

[Miquel Raynal]

Hi Herve,

herve.codina@bootlin.com wrote on Fri, 19 Nov 2021 08:39:06 +0100:

> NAND_OP_CMD() expect a delay parameter in nanoseconds.

expects

> The delay value is wrongly given in milliseconds.
> 
> Fix the conversion macro used in order to set this
> delay in nanoseconds.
> 

I believe you still miss the Fixes: tag here :)

> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
> Changes v1 -> v2:
> - Commit log reword
> 
>  drivers/mtd/nand/raw/nand_base.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 3d6c6e880520..5c6b065837ef 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -1837,7 +1837,7 @@ int nand_erase_op(struct nand_chip *chip, unsigned int eraseblock)
>  			NAND_OP_CMD(NAND_CMD_ERASE1, 0),
>  			NAND_OP_ADDR(2, addrs, 0),
>  			NAND_OP_CMD(NAND_CMD_ERASE2,
> -				    NAND_COMMON_TIMING_MS(conf, tWB_max)),
> +				    NAND_COMMON_TIMING_NS(conf, tWB_max)),
>  			NAND_OP_WAIT_RDY(NAND_COMMON_TIMING_MS(conf, tBERS_max),
>  					 0),
>  		};

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 4/4] mtd: rawnand: fsmc: Fix timing computation

[Miquel Raynal]

Hi Herve,

herve.codina@bootlin.com wrote on Fri, 19 Nov 2021 08:39:09 +0100:

> Under certain circumstances, the timing settings calculated by
> the FSMC NAND controller driver were inaccurate.
> These settings lead to incorrect data reads or fallback to

led to

oneOf:
  - fallbacks to timing mode 0
  - fallbacks to mode 0
but "mode X timing" does not look good :) (same below, it applies all
over this commit log)

> mode 0 timings depending on NAND chip used.

depending on the NAND chip used

> 
> The timing computation did not take into account the following
> constraint given in SPEAr3xx reference manual:
>   twait >= tCEA - tset*TCLK + TOUTDEL + TINDEL

See below for the style request (this really is nitpicking, I'm sorry)

> 
> Enhance the timings calculation by taking into account this
> additional constraint.
> 
> The change has no impact on slow timing mode such as mode 0.

This change?
slow timing modes

> Indeed, on mode 0 timing, computed values are the same with and
> without the patch.
> 
> The NAND that previously used the mode 0 fallback timing is
> working in mode 3 timing.

I believe we should be more generic:
"NANDs which previously stayed in mode 0 because of ... can now work
at higher speeds and NANDs which were not working at all because of the
corrupted data returned interact at high speeds without troubles.

Overall improvement on a <flash name/type>:"

> The read/writes accesses performances (flash_speed tool result)
> are the following:
>                         mode0       mode3
> eraseblock write speed  3220 KiB/s  4511 KiB/s
> eraseblock read speed   4491 KiB/s  7529 KiB/s
> 
> The NAND where data reads were previously incorrect are correct
> and this NAND is also working at mode 3 timing.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
> Changes v1 to v2:
> - Commit log reword
> - Added performance details in commit log
> - Used #define for TOUTDEL and TINDEL and
>   Fixed coding style
> - Used max3()
> 
>  drivers/mtd/nand/raw/fsmc_nand.c | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/fsmc_nand.c b/drivers/mtd/nand/raw/fsmc_nand.c
> index 0a6c9ef0ea8b..56b9da252346 100644
> --- a/drivers/mtd/nand/raw/fsmc_nand.c
> +++ b/drivers/mtd/nand/raw/fsmc_nand.c
> @@ -94,6 +94,14 @@
>  
>  #define FSMC_BUSY_WAIT_TIMEOUT	(1 * HZ)
>  
> +/*
> + * According to SPEAr300 Reference Manual (RM0082)
> + *  TOUDEL = 7ns (Output delay from the flip-flops to the board)
> + *  TINDEL = 5ns (Input delay from the board to the flipflop)
> + */
> +#define TOUTDEL	7000
> +#define TINDEL	5000
> +
>  struct fsmc_nand_timings {
>  	u8 tclr;
>  	u8 tar;
> @@ -278,7 +286,7 @@ static int fsmc_calc_timings(struct fsmc_nand_data *host,
>  {
>  	unsigned long hclk = clk_get_rate(host->clk);
>  	unsigned long hclkn = NSEC_PER_SEC / hclk;
> -	u32 thiz, thold, twait, tset;
> +	u32 thiz, thold, twait, tset, twait_min;
>  
>  	if (sdrt->tRC_min < 30000)
>  		return -EOPNOTSUPP;
> @@ -310,13 +318,6 @@ static int fsmc_calc_timings(struct fsmc_nand_data *host,
>  	else if (tims->thold > FSMC_THOLD_MASK)
>  		tims->thold = FSMC_THOLD_MASK;
>  
> -	twait = max(sdrt->tRP_min, sdrt->tWP_min);
> -	tims->twait = DIV_ROUND_UP(twait / 1000, hclkn) - 1;
> -	if (tims->twait == 0)
> -		tims->twait = 1;
> -	else if (tims->twait > FSMC_TWAIT_MASK)
> -		tims->twait = FSMC_TWAIT_MASK;
> -
>  	tset = max(sdrt->tCS_min - sdrt->tWP_min,
>  		   sdrt->tCEA_max - sdrt->tREA_max);
>  	tims->tset = DIV_ROUND_UP(tset / 1000, hclkn) - 1;
> @@ -325,6 +326,21 @@ static int fsmc_calc_timings(struct fsmc_nand_data *host,
>  	else if (tims->tset > FSMC_TSET_MASK)
>  		tims->tset = FSMC_TSET_MASK;
>  
> +	/*
> +	 * According to SPEAr300 Reference Manual (RM0082) which gives more
> +	 * information related to FSMSC timings than the SPEAr600 one (RM0305),
> +	 *   twait >= tCEA - tset*TCLK + TOUTDEL + TINDEL

It is certainly best if in plain text we follow the same spacing
convention than in the code: (tset * TCLK). Same applies to your commit
log.

> +	 */
> +	twait_min = sdrt->tCEA_max - ((tims->tset + 1) * hclkn * 1000)
> +		    + TOUTDEL + TINDEL;
> +	twait = max3(sdrt->tRP_min, sdrt->tWP_min, twait_min);
> +
> +	tims->twait = DIV_ROUND_UP(twait / 1000, hclkn) - 1;
> +	if (tims->twait == 0)
> +		tims->twait = 1;
> +	else if (tims->twait > FSMC_TWAIT_MASK)
> +		tims->twait = FSMC_TWAIT_MASK;
> +
>  	return 0;
>  }
>  

Otherwise looks good.

Thanks,
Miquèl

Re: [PATCH v2 4/4] mtd: rawnand: fsmc: Fix timing computation

[Miquel Raynal]

Hi Herve,

herve.codina@bootlin.com wrote on Fri, 19 Nov 2021 08:39:09 +0100:

> Under certain circumstances, the timing settings calculated by
> the FSMC NAND controller driver were inaccurate.
> These settings lead to incorrect data reads or fallback to

led to

oneOf:
  - fallbacks to timing mode 0
  - fallbacks to mode 0
but "mode X timing" does not look good :) (same below, it applies all
over this commit log)

> mode 0 timings depending on NAND chip used.

depending on the NAND chip used

> 
> The timing computation did not take into account the following
> constraint given in SPEAr3xx reference manual:
>   twait >= tCEA - tset*TCLK + TOUTDEL + TINDEL

See below for the style request (this really is nitpicking, I'm sorry)

> 
> Enhance the timings calculation by taking into account this
> additional constraint.
> 
> The change has no impact on slow timing mode such as mode 0.

This change?
slow timing modes

> Indeed, on mode 0 timing, computed values are the same with and
> without the patch.
> 
> The NAND that previously used the mode 0 fallback timing is
> working in mode 3 timing.

I believe we should be more generic:
"NANDs which previously stayed in mode 0 because of ... can now work
at higher speeds and NANDs which were not working at all because of the
corrupted data returned interact at high speeds without troubles.

Overall improvement on a <flash name/type>:"

> The read/writes accesses performances (flash_speed tool result)
> are the following:
>                         mode0       mode3
> eraseblock write speed  3220 KiB/s  4511 KiB/s
> eraseblock read speed   4491 KiB/s  7529 KiB/s
> 
> The NAND where data reads were previously incorrect are correct
> and this NAND is also working at mode 3 timing.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
> Changes v1 to v2:
> - Commit log reword
> - Added performance details in commit log
> - Used #define for TOUTDEL and TINDEL and
>   Fixed coding style
> - Used max3()
> 
>  drivers/mtd/nand/raw/fsmc_nand.c | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/fsmc_nand.c b/drivers/mtd/nand/raw/fsmc_nand.c
> index 0a6c9ef0ea8b..56b9da252346 100644
> --- a/drivers/mtd/nand/raw/fsmc_nand.c
> +++ b/drivers/mtd/nand/raw/fsmc_nand.c
> @@ -94,6 +94,14 @@
>  
>  #define FSMC_BUSY_WAIT_TIMEOUT	(1 * HZ)
>  
> +/*
> + * According to SPEAr300 Reference Manual (RM0082)
> + *  TOUDEL = 7ns (Output delay from the flip-flops to the board)
> + *  TINDEL = 5ns (Input delay from the board to the flipflop)
> + */
> +#define TOUTDEL	7000
> +#define TINDEL	5000
> +
>  struct fsmc_nand_timings {
>  	u8 tclr;
>  	u8 tar;
> @@ -278,7 +286,7 @@ static int fsmc_calc_timings(struct fsmc_nand_data *host,
>  {
>  	unsigned long hclk = clk_get_rate(host->clk);
>  	unsigned long hclkn = NSEC_PER_SEC / hclk;
> -	u32 thiz, thold, twait, tset;
> +	u32 thiz, thold, twait, tset, twait_min;
>  
>  	if (sdrt->tRC_min < 30000)
>  		return -EOPNOTSUPP;
> @@ -310,13 +318,6 @@ static int fsmc_calc_timings(struct fsmc_nand_data *host,
>  	else if (tims->thold > FSMC_THOLD_MASK)
>  		tims->thold = FSMC_THOLD_MASK;
>  
> -	twait = max(sdrt->tRP_min, sdrt->tWP_min);
> -	tims->twait = DIV_ROUND_UP(twait / 1000, hclkn) - 1;
> -	if (tims->twait == 0)
> -		tims->twait = 1;
> -	else if (tims->twait > FSMC_TWAIT_MASK)
> -		tims->twait = FSMC_TWAIT_MASK;
> -
>  	tset = max(sdrt->tCS_min - sdrt->tWP_min,
>  		   sdrt->tCEA_max - sdrt->tREA_max);
>  	tims->tset = DIV_ROUND_UP(tset / 1000, hclkn) - 1;
> @@ -325,6 +326,21 @@ static int fsmc_calc_timings(struct fsmc_nand_data *host,
>  	else if (tims->tset > FSMC_TSET_MASK)
>  		tims->tset = FSMC_TSET_MASK;
>  
> +	/*
> +	 * According to SPEAr300 Reference Manual (RM0082) which gives more
> +	 * information related to FSMSC timings than the SPEAr600 one (RM0305),
> +	 *   twait >= tCEA - tset*TCLK + TOUTDEL + TINDEL

It is certainly best if in plain text we follow the same spacing
convention than in the code: (tset * TCLK). Same applies to your commit
log.

> +	 */
> +	twait_min = sdrt->tCEA_max - ((tims->tset + 1) * hclkn * 1000)
> +		    + TOUTDEL + TINDEL;
> +	twait = max3(sdrt->tRP_min, sdrt->tWP_min, twait_min);
> +
> +	tims->twait = DIV_ROUND_UP(twait / 1000, hclkn) - 1;
> +	if (tims->twait == 0)
> +		tims->twait = 1;
> +	else if (tims->twait > FSMC_TWAIT_MASK)
> +		tims->twait = FSMC_TWAIT_MASK;
> +
>  	return 0;
>  }
>  

Otherwise looks good.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 2/4] mtd: rawnand: Fix nand_choose_best_timings() on unsupported interface

[Miquel Raynal]

Hi Herve,

herve.codina@bootlin.com wrote on Fri, 19 Nov 2021 08:39:07 +0100:

> When the NV-DDR interface is not supported by the NAND chip,
> the value of onfi->nvddr_timing_modes is 0. In this case,
> the best_mode variable value in nand_choose_best_nvddr_timings()
> is -1. The last for-loop is skipped and the function returns an
> uninitialized value.

Actually is not the first time this uninitialized value triggers bells
but, while I think in the SDR path it is still not needed,in the DDR
patch you are right that something is missing.

> If this returned value is 0, the nand_choose_best_sdr_timings()
> is not executed and no 'best timing' are set. This leads the host
> controller and the NAND chip working at default mode 0 timing
> even if a better timing can be used.
> 
> Fix this uninitialzed returned value.

typo                ^

> 
> nand_choose_best_sdr_timings() is pretty similar to
> nand_choose_best_nvddr_timings(). Even if onfi->sdr_timing_modes
> should never be seen as 0, nand_choose_best_sdr_timings() returned
> value is fixed.

I still don't think it really needed by let's keep so  everyone
(including robots) is happy :)

> 
> Fixes: a9ecc8c814e9 ("mtd: rawnand: Choose the best timings, NV-DDR included")
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
> Changes v1 to v2:
> - New patch in v2 series
> 
>  drivers/mtd/nand/raw/nand_base.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 5c6b065837ef..a130320de412 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -926,7 +926,7 @@ int nand_choose_best_sdr_timings(struct nand_chip *chip,
>  				 struct nand_sdr_timings *spec_timings)
>  {
>  	const struct nand_controller_ops *ops = chip->controller->ops;
> -	int best_mode = 0, mode, ret;
> +	int best_mode = 0, mode, ret = -EOPNOTSUPP;
>  
>  	iface->type = NAND_SDR_IFACE;
>  
> @@ -977,7 +977,7 @@ int nand_choose_best_nvddr_timings(struct nand_chip *chip,
>  				   struct nand_nvddr_timings *spec_timings)
>  {
>  	const struct nand_controller_ops *ops = chip->controller->ops;
> -	int best_mode = 0, mode, ret;
> +	int best_mode = 0, mode, ret = -EOPNOTSUPP;
>  
>  	iface->type = NAND_NVDDR_IFACE;
>  


Thanks,
Miquèl

Re: [PATCH v2 2/4] mtd: rawnand: Fix nand_choose_best_timings() on unsupported interface

[Miquel Raynal]

Hi Herve,

herve.codina@bootlin.com wrote on Fri, 19 Nov 2021 08:39:07 +0100:

> When the NV-DDR interface is not supported by the NAND chip,
> the value of onfi->nvddr_timing_modes is 0. In this case,
> the best_mode variable value in nand_choose_best_nvddr_timings()
> is -1. The last for-loop is skipped and the function returns an
> uninitialized value.

Actually is not the first time this uninitialized value triggers bells
but, while I think in the SDR path it is still not needed,in the DDR
patch you are right that something is missing.

> If this returned value is 0, the nand_choose_best_sdr_timings()
> is not executed and no 'best timing' are set. This leads the host
> controller and the NAND chip working at default mode 0 timing
> even if a better timing can be used.
> 
> Fix this uninitialzed returned value.

typo                ^

> 
> nand_choose_best_sdr_timings() is pretty similar to
> nand_choose_best_nvddr_timings(). Even if onfi->sdr_timing_modes
> should never be seen as 0, nand_choose_best_sdr_timings() returned
> value is fixed.

I still don't think it really needed by let's keep so  everyone
(including robots) is happy :)

> 
> Fixes: a9ecc8c814e9 ("mtd: rawnand: Choose the best timings, NV-DDR included")
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
> Changes v1 to v2:
> - New patch in v2 series
> 
>  drivers/mtd/nand/raw/nand_base.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 5c6b065837ef..a130320de412 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -926,7 +926,7 @@ int nand_choose_best_sdr_timings(struct nand_chip *chip,
>  				 struct nand_sdr_timings *spec_timings)
>  {
>  	const struct nand_controller_ops *ops = chip->controller->ops;
> -	int best_mode = 0, mode, ret;
> +	int best_mode = 0, mode, ret = -EOPNOTSUPP;
>  
>  	iface->type = NAND_SDR_IFACE;
>  
> @@ -977,7 +977,7 @@ int nand_choose_best_nvddr_timings(struct nand_chip *chip,
>  				   struct nand_nvddr_timings *spec_timings)
>  {
>  	const struct nand_controller_ops *ops = chip->controller->ops;
> -	int best_mode = 0, mode, ret;
> +	int best_mode = 0, mode, ret = -EOPNOTSUPP;
>  
>  	iface->type = NAND_NVDDR_IFACE;
>  


Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/