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

[PATCH 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 first patch which fixes an operation delay setting in nand base.

Details are given in each patches.

Best regards,
Herve Codina

Herve Codina (4):
  mtd: rawnand: Fix nand_erase_op delay
  mtd: rawnand: fsmc: Force to use 8 bits access when expected
  mtd: rawnand: fsmc: Take instruction delay into account
  mtd: rawnand: fsmc: Fix timing computation

 drivers/mtd/nand/raw/fsmc_nand.c | 46 ++++++++++++++++++++++----------
 drivers/mtd/nand/raw/nand_base.c |  2 +-
 2 files changed, 33 insertions(+), 15 deletions(-)

-- 
2.31.1

[PATCH 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.

This patch fixes the conversion macro used in order
to set this delay in nanoseconds.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 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 2/4] mtd: rawnand: fsmc: Force to use 8 bits access when expected

[Herve Codina]

Some data transfers are expected on 8 bits by the nand core.
The fsmc driver did not check this constraint and these transfers
can be done on 32 bits depending on buffer alignment and transfers
data size.

This patch ensures that these transfers will be 8bits transfers in
all cases.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/mtd/nand/raw/fsmc_nand.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/raw/fsmc_nand.c b/drivers/mtd/nand/raw/fsmc_nand.c
index 658f0cbe7ce8..7f057cfee6c4 100644
--- a/drivers/mtd/nand/raw/fsmc_nand.c
+++ b/drivers/mtd/nand/raw/fsmc_nand.c
@@ -540,12 +540,12 @@ static int dma_xfer(struct fsmc_nand_data *host, void *buffer, int len,
  * @len:	number of bytes to write
  */
 static void fsmc_write_buf(struct fsmc_nand_data *host, const u8 *buf,
-			   int len)
+			   int len, bool force_8bit)
 {
 	int i;
 
 	if (IS_ALIGNED((uintptr_t)buf, sizeof(u32)) &&
-	    IS_ALIGNED(len, sizeof(u32))) {
+	    IS_ALIGNED(len, sizeof(u32)) && !force_8bit) {
 		u32 *p = (u32 *)buf;
 
 		len = len >> 2;
@@ -563,12 +563,13 @@ static void fsmc_write_buf(struct fsmc_nand_data *host, const u8 *buf,
  * @buf:	buffer to store date
  * @len:	number of bytes to read
  */
-static void fsmc_read_buf(struct fsmc_nand_data *host, u8 *buf, int len)
+static void fsmc_read_buf(struct fsmc_nand_data *host, u8 *buf, int len,
+			  bool force_8bit)
 {
 	int i;
 
 	if (IS_ALIGNED((uintptr_t)buf, sizeof(u32)) &&
-	    IS_ALIGNED(len, sizeof(u32))) {
+	    IS_ALIGNED(len, sizeof(u32)) && !force_8bit) {
 		u32 *p = (u32 *)buf;
 
 		len = len >> 2;
@@ -646,7 +647,8 @@ static int fsmc_exec_op(struct nand_chip *chip, const struct nand_operation *op,
 						  instr->ctx.data.len);
 			else
 				fsmc_read_buf(host, instr->ctx.data.buf.in,
-					      instr->ctx.data.len);
+					      instr->ctx.data.len,
+					      instr->ctx.data.force_8bit);
 			break;
 
 		case NAND_OP_DATA_OUT_INSTR:
@@ -656,7 +658,8 @@ static int fsmc_exec_op(struct nand_chip *chip, const struct nand_operation *op,
 						   instr->ctx.data.len);
 			else
 				fsmc_write_buf(host, instr->ctx.data.buf.out,
-					       instr->ctx.data.len);
+					       instr->ctx.data.len,
+					       instr->ctx.data.force_8bit);
 			break;
 
 		case NAND_OP_WAITRDY_INSTR:
-- 
2.31.1

Re: [PATCH 2/4] mtd: rawnand: fsmc: Force to use 8 bits access when expected

[Miquel Raynal]

Hi Hervé,

herve.codina@bootlin.com wrote on Fri, 12 Nov 2021 15:38:53 +0100:

> Some data transfers are expected on 8 bits by the nand core.
> The fsmc driver did not check this constraint and these transfers
> can be done on 32 bits depending on buffer alignment and transfers
> data size.
> 
> This patch ensures that these transfers will be 8bits transfers in
> all cases.

I believe there is a misunderstanding here: NAND buses -between the
NAND controller and the NAND chip- are either 8-bit or 16-bit wide and
the amount of bytes that you will retrieve per register read is not
related to it.

When the controller supports 16-bit accesses, there are certain
operations that must be performed using only the lowest 8 bits of the
NAND bus, such as reading a status [1]. In this case, the controller
must have a way to disable the 16-bit mode temporarily. See [2] and [3]
for an example. Reading with readb() or readl() will IMHO not impact the
amount of data lines used for the operation.

> Signed-off-by: Herve Codina <herve.codina@bootlin.com>

[1] https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_base.c#L673
[2] Marvell NAND controller can change the used width of the bus
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/marvell_nand.c#L1777
[3] ... while still doing 32-bit accesses
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/marvell_nand.c#L906

Thanks,
Miquèl

> ---
>  drivers/mtd/nand/raw/fsmc_nand.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/fsmc_nand.c b/drivers/mtd/nand/raw/fsmc_nand.c
> index 658f0cbe7ce8..7f057cfee6c4 100644
> --- a/drivers/mtd/nand/raw/fsmc_nand.c
> +++ b/drivers/mtd/nand/raw/fsmc_nand.c
> @@ -540,12 +540,12 @@ static int dma_xfer(struct fsmc_nand_data *host, void *buffer, int len,
>   * @len:	number of bytes to write
>   */
>  static void fsmc_write_buf(struct fsmc_nand_data *host, const u8 *buf,
> -			   int len)
> +			   int len, bool force_8bit)
>  {
>  	int i;
>  
>  	if (IS_ALIGNED((uintptr_t)buf, sizeof(u32)) &&
> -	    IS_ALIGNED(len, sizeof(u32))) {
> +	    IS_ALIGNED(len, sizeof(u32)) && !force_8bit) {
>  		u32 *p = (u32 *)buf;
>  
>  		len = len >> 2;
> @@ -563,12 +563,13 @@ static void fsmc_write_buf(struct fsmc_nand_data *host, const u8 *buf,
>   * @buf:	buffer to store date
>   * @len:	number of bytes to read
>   */
> -static void fsmc_read_buf(struct fsmc_nand_data *host, u8 *buf, int len)
> +static void fsmc_read_buf(struct fsmc_nand_data *host, u8 *buf, int len,
> +			  bool force_8bit)
>  {
>  	int i;
>  
>  	if (IS_ALIGNED((uintptr_t)buf, sizeof(u32)) &&
> -	    IS_ALIGNED(len, sizeof(u32))) {
> +	    IS_ALIGNED(len, sizeof(u32)) && !force_8bit) {
>  		u32 *p = (u32 *)buf;
>  
>  		len = len >> 2;
> @@ -646,7 +647,8 @@ static int fsmc_exec_op(struct nand_chip *chip, const struct nand_operation *op,
>  						  instr->ctx.data.len);
>  			else
>  				fsmc_read_buf(host, instr->ctx.data.buf.in,
> -					      instr->ctx.data.len);
> +					      instr->ctx.data.len,
> +					      instr->ctx.data.force_8bit);
>  			break;
>  
>  		case NAND_OP_DATA_OUT_INSTR:
> @@ -656,7 +658,8 @@ static int fsmc_exec_op(struct nand_chip *chip, const struct nand_operation *op,
>  						   instr->ctx.data.len);
>  			else
>  				fsmc_write_buf(host, instr->ctx.data.buf.out,
> -					       instr->ctx.data.len);
> +					       instr->ctx.data.len,
> +					       instr->ctx.data.force_8bit);
>  			break;
>  
>  		case NAND_OP_WAITRDY_INSTR:

Re: [PATCH 2/4] mtd: rawnand: fsmc: Force to use 8 bits access when expected

[Herve Codina]

Hi,

On Fri, 12 Nov 2021 16:38:59 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Hervé,
> 
> herve.codina@bootlin.com wrote on Fri, 12 Nov 2021 15:38:53 +0100:
> 
> > Some data transfers are expected on 8 bits by the nand core.
> > The fsmc driver did not check this constraint and these transfers
> > can be done on 32 bits depending on buffer alignment and transfers
> > data size.
> > 
> > This patch ensures that these transfers will be 8bits transfers in
> > all cases.  
> 
> I believe there is a misunderstanding here: NAND buses -between the
> NAND controller and the NAND chip- are either 8-bit or 16-bit wide and
> the amount of bytes that you will retrieve per register read is not
> related to it.
> 
> When the controller supports 16-bit accesses, there are certain
> operations that must be performed using only the lowest 8 bits of the
> NAND bus, such as reading a status [1]. In this case, the controller
> must have a way to disable the 16-bit mode temporarily. See [2] and [3]
> for an example. Reading with readb() or readl() will IMHO not impact the
> amount of data lines used for the operation.
> 

Indeed, I misunderstood the force_8bit usage.
This patch is not needed and will be simply removed in v2 series.

Thanks,
Hervé

[PATCH 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.

This patch adds this waiting delay in the FSMC nand
controller driver.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 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 7f057cfee6c4..bff09219ce3a 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>
@@ -667,6 +668,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 4/4] mtd: rawnand: fsmc: Fix timing computation

[Herve Codina]

The timing setting were incorrect on some nands leading to a
fallback to mode 0 timing on some Micron nand or to incorrect
data reads on some Winbond NAND.

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

This patch adds this constraint and fixes the issues on both
nands having the both nands working at mode 3 timing.
The change has no impact on slower timing mode such as mode 0.
Indeed, on mode 0 timing, computed values are the same with and
without the patch.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/mtd/nand/raw/fsmc_nand.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/nand/raw/fsmc_nand.c b/drivers/mtd/nand/raw/fsmc_nand.c
index bff09219ce3a..a3aa66f30869 100644
--- a/drivers/mtd/nand/raw/fsmc_nand.c
+++ b/drivers/mtd/nand/raw/fsmc_nand.c
@@ -278,7 +278,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, tmp;
 
 	if (sdrt->tRC_min < 30000)
 		return -EOPNOTSUPP;
@@ -310,13 +310,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 +318,24 @@ static int fsmc_calc_timings(struct fsmc_nand_data *host,
 	else if (tims->tset > FSMC_TSET_MASK)
 		tims->tset = FSMC_TSET_MASK;
 
+	twait = max(sdrt->tRP_min, sdrt->tWP_min);
+
+	/* 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
+	 * With TOUTDEL = 7ns (Output delay from the flip-flops to the board)
+	 * and TINDEL = 5ns (Input delay from the board to the flipflop)
+	 */
+	tmp = sdrt->tCEA_max - (tims->tset + 1)*hclkn*1000 + 7000 + 5000;
+	if (twait < tmp)
+		twait = tmp;
+
+	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

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

[Miquel Raynal]

Hi Herve,

herve.codina@bootlin.com wrote on Fri, 12 Nov 2021 15:38:55 +0100:

> The timing setting were incorrect on some nands leading to a
> fallback to mode 0 timing on some Micron nand or to incorrect
> data reads on some Winbond NAND.

In general I prefer when acronyms use upper cases (NANDs, MTD, etc).
Please also do the change in the other commits.

Saying "timing setting were incorrect on some nands" is inaccurate. I
think we can clearly state that "Under certain circumstances, the
timing settings calculated by the FSMC NAND controller driver were
inaccurate." (no need to precise a NAND device here).

> The timing computation did not take into account the following
> constraint given in SPEAr3xx reference manual:
>   twait >= tCEA - tset*TCLK + TOUTDEL + TINDEL
> 
> This patch adds this constraint and fixes the issues on both

We generally use the declarative tense here, such as "Add this
constraint and fixes..." even though I would happily reformulate with
something like "Enhance the timings calculation by taking into account
this additional constraint."

And then you can provide your results, how it behaved with the two
NANDs and what improvement you bring with this additional calculation.

> nands having the both nands working at mode 3 timing.
> The change has no impact on slower timing mode such as mode 0.
> Indeed, on mode 0 timing, computed values are the same with and
> without the patch.

A few visual changes explained below otherwise the logic is fine.

> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  drivers/mtd/nand/raw/fsmc_nand.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/fsmc_nand.c b/drivers/mtd/nand/raw/fsmc_nand.c
> index bff09219ce3a..a3aa66f30869 100644
> --- a/drivers/mtd/nand/raw/fsmc_nand.c
> +++ b/drivers/mtd/nand/raw/fsmc_nand.c
> @@ -278,7 +278,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, tmp;
>  
>  	if (sdrt->tRC_min < 30000)
>  		return -EOPNOTSUPP;
> @@ -310,13 +310,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 +318,24 @@ static int fsmc_calc_timings(struct fsmc_nand_data *host,
>  	else if (tims->tset > FSMC_TSET_MASK)
>  		tims->tset = FSMC_TSET_MASK;
>  
> +	twait = max(sdrt->tRP_min, sdrt->tWP_min);
> +
> +	/* According to SPEAr300 Reference Manual (RM0082) which gives more

           ^
Should be:

	/*
	 * According to...

> +	 * information related to FSMSC timings than the SPEAr600 one (RM0305),
> +	 *   twait >= tCEA - tset*TCLK + TOUTDEL + TINDEL
> +	 * With TOUTDEL = 7ns (Output delay from the flip-flops to the board)
> +	 * and TINDEL = 5ns (Input delay from the board to the flipflop)

These two information should be placed close to TOUTDEL and TINDEL
definitions (see my comment below).

> +	 */
> +	tmp = sdrt->tCEA_max - (tims->tset + 1)*hclkn*1000 + 7000 + 5000;

                                               ^
The style here is wrong, you need spaces before and after a "*".

Please also enclose these two multiplications with parenthesis to make
it clear that you do not intend to multiply 7000 nor 5000.

Finally, do not use open coded values, please define them at the top so
that your code matches your commit message (using TOUTDEL and TINDEL).
You can even add a comment explaining from where you got these values.

> +	if (twait < tmp)
> +		twait = tmp;

I don't like the tmp naming, you could call this variable twait_min and
use a min() calculation here to fit the manual better.

> +
> +	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;
>  }
>  


Thanks,
Miquèl