[PATCH v4] mtd:nor:timeout:fix do_write_buffer() timeout error

[PATCH v4] mtd:nor:timeout:fix do_write_buffer() timeout error

[Bean Huo]

For some Norflashes,the size of the buffer program has been
increased from 256 bytes to 512 bytes,2ms maximum timeout can
not adapt to all the different vendor's norflash.There maximum
timeout information in the CFI area,so the best way is to choose
the result calculated according to timeout field of struct cfi_ident
that probed from norflash's CFI aera.This is also a standard defined
by CFI.If haven't probed this information,or smaller than 2000us,then
specify a defualt value 2000us.

Tested with Micron JS28F512M29EWx and Micron MT28EW512ABA flash devices.

Signed-off-by: bean huo <beanhuo@outlook.com>
---
changes
	v1->v2:Deleted unused parameters in this patch 
		(word_write_time_max and erase_time_max).Using usecs_to_jiffies
		instead of msecs_to_jiffies for convert timeout value into jiffies.
	v2->v3:Removed unnecessary messages form comments and deleted trailing whitespace.
	V3->v4:If haven't probed timeout information,or smaller than 2000us,specify default
	       2000us.

 drivers/mtd/chips/cfi_cmdset_0002.c |   28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index e21fde9..c248a8c 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -628,6 +628,24 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
 		cfi->chips[i].word_write_time = 1<<cfi->cfiq->WordWriteTimeoutTyp;
 		cfi->chips[i].buffer_write_time = 1<<cfi->cfiq->BufWriteTimeoutTyp;
 		cfi->chips[i].erase_time = 1<<cfi->cfiq->BlockEraseTimeoutTyp;
+		/*
+		 * First calculate the timeout max according to timeout
+		 * field of struct cfi_ident that probed from chip's CFI
+		 * aera.If haven't probed this information,or smaller than
+		 * 2000us,we will specify defualt value 2000us,and the time
+		 * unit is us.
+		 */
+		if (cfi->cfiq->BufWriteTimeoutTyp &&
+			cfi->cfiq->BufWriteTimeoutMax){
+			cfi->chips[i].buffer_write_time_max =
+				1<<(cfi->cfiq->BufWriteTimeoutTyp +
+					cfi->cfiq->BufWriteTimeoutMax);
+			} else {
+				cfi->chips[i].buffer_write_time_max = 0;
+				}
+		cfi->chips[i].buffer_write_time_max =
+			((cfi->chips[i].buffer_write_time_max>= 2000)
+			 ? cfi->chips[i].buffer_write_time_max : 2000);
 		cfi->chips[i].ref_point_counter = 0;
 		init_waitqueue_head(&(cfi->chips[i].wq));
 	}
@@ -1462,8 +1480,14 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
 {
 	struct cfi_private *cfi = map->fldrv_priv;
 	unsigned long timeo = jiffies + HZ;
-	/* see comments in do_write_oneword() regarding uWriteTimeo. */
-	unsigned long uWriteTimeout = ( HZ / 1000 ) + 1;
+	/*
+	 * Use the result that calculated according to timeout field
+	 * of struct cfi_ident that probed from norflash's CFI aera.
+	 * See more comments in cfi_cmdset_0002().uWriteTimeout is
+	 * used for timeout step,it must be concerted into jiffies.
+	 */
+	unsigned long uWriteTimeout =
+				usecs_to_jiffies(chip->buffer_write_time_max);
 	int ret = -EIO;
 	unsigned long cmd_adr;
 	int z, words;
-- 
1.7.9.5 		 	   		  

[PATCH v5] mtd:nor:timeout:fix do_write_buffer() timeout error

[Bean Huo]

For some Norflashes,the size of the buffer program has been
increased from 256 bytes to 512 bytes,2ms maximum timeout can
not adapt to all the different vendor's norflash.There maximum
timeout information in the CFI area,so the best way is to choose
the result calculated according to timeout field of struct cfi_ident
that probed from norflash's CFI aera.This is also a standard defined
by CFI.If haven't probed this information,or smaller than 2000us,then
specify a defualt value 2000us.

Tested with Micron JS28F512M29EWx and Micron MT28EW512ABA flash devices.

Signed-off-by: bean huo <beanhuo@outlook.com>
---
changes
	v1->v2:Deleted unused parameters in this patch 
		(word_write_time_max and erase_time_max).Using usecs_to_jiffies
		instead of msecs_to_jiffies for convert timeout value into jiffies.
	v2->v3:Removed unnecessary messages form comments and deleted trailing whitespace.
	V3->v4:If haven't probed timeout information,or smaller than 2000us,specify default
	       2000us.
	v4->v5:add one whitespace
 drivers/mtd/chips/cfi_cmdset_0002.c |   28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index e21fde9..c248a8c 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -628,6 +628,24 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
 		cfi->chips[i].word_write_time = 1<<cfi->cfiq->WordWriteTimeoutTyp;
 		cfi->chips[i].buffer_write_time = 1<<cfi->cfiq->BufWriteTimeoutTyp;
 		cfi->chips[i].erase_time = 1<<cfi->cfiq->BlockEraseTimeoutTyp;
+		/*
+		 * First calculate the timeout max according to timeout
+		 * field of struct cfi_ident that probed from chip's CFI
+		 * aera.If haven't probed this information,or smaller than
+		 * 2000us,we will specify defualt value 2000us,and the time
+		 * unit is us.
+		 */
+		if (cfi->cfiq->BufWriteTimeoutTyp &&
+			cfi->cfiq->BufWriteTimeoutMax){
+			cfi->chips[i].buffer_write_time_max =
+				1<<(cfi->cfiq->BufWriteTimeoutTyp +
+					cfi->cfiq->BufWriteTimeoutMax);
+			} else {
+				cfi->chips[i].buffer_write_time_max = 0;
+				}
+		cfi->chips[i].buffer_write_time_max =
+			((cfi->chips[i].buffer_write_time_max>= 2000)
+			 ? cfi->chips[i].buffer_write_time_max : 2000);
 		cfi->chips[i].ref_point_counter = 0;
 		init_waitqueue_head(&(cfi->chips[i].wq));
 	}
@@ -1462,8 +1480,14 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
 {
 	struct cfi_private *cfi = map->fldrv_priv;
 	unsigned long timeo = jiffies + HZ;
-	/* see comments in do_write_oneword() regarding uWriteTimeo. */
-	unsigned long uWriteTimeout = ( HZ / 1000 ) + 1;
+	/*
+	 * Use the result that calculated according to timeout field
+	 * of struct cfi_ident that probed from norflash's CFI aera.
+	 * See more comments in cfi_cmdset_0002().uWriteTimeout is
+	 * used for timeout step,it must be concerted into jiffies.
+	 */
+	unsigned long uWriteTimeout =
+				usecs_to_jiffies(chip->buffer_write_time_max);
 	int ret = -EIO;
 	unsigned long cmd_adr;
 	int z, words;
-- 
1.7.9.5 		 	   		  

Re: [PATCH v5] mtd:nor:timeout:fix do_write_buffer() timeout error

[Jingoo Han]

On Wednesday, July 16, 2014 4:19 PM, Bean Huo wrote:
> 
> For some Norflashes,the size of the buffer program has been
> increased from 256 bytes to 512 bytes,2ms maximum timeout can
> not adapt to all the different vendor's norflash.There maximum
> timeout information in the CFI area,so the best way is to choose
> the result calculated according to timeout field of struct cfi_ident
> that probed from norflash's CFI aera.This is also a standard defined
> by CFI.If haven't probed this information,or smaller than 2000us,then
> specify a defualt value 2000us.
> 
> Tested with Micron JS28F512M29EWx and Micron MT28EW512ABA flash devices.
> 
> Signed-off-by: bean huo <beanhuo@outlook.com>
> ---
> changes
> 	v1->v2:Deleted unused parameters in this patch
> 		(word_write_time_max and erase_time_max).Using usecs_to_jiffies
> 		instead of msecs_to_jiffies for convert timeout value into jiffies.
> 	v2->v3:Removed unnecessary messages form comments and deleted trailing whitespace.
> 	V3->v4:If haven't probed timeout information,or smaller than 2000us,specify default
> 	       2000us.
> 	v4->v5:add one whitespace
>  drivers/mtd/chips/cfi_cmdset_0002.c |   28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index e21fde9..c248a8c 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -628,6 +628,24 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
>  		cfi->chips[i].word_write_time = 1<<cfi->cfiq->WordWriteTimeoutTyp;
>  		cfi->chips[i].buffer_write_time = 1<<cfi->cfiq->BufWriteTimeoutTyp;
>  		cfi->chips[i].erase_time = 1<<cfi->cfiq->BlockEraseTimeoutTyp;
> +		/*
> +		 * First calculate the timeout max according to timeout
> +		 * field of struct cfi_ident that probed from chip's CFI
> +		 * aera.If haven't probed this information,or smaller than
> +		 * 2000us,we will specify defualt value 2000us,and the time
> +		 * unit is us.
> +		 */
> +		if (cfi->cfiq->BufWriteTimeoutTyp &&
> +			cfi->cfiq->BufWriteTimeoutMax){
> +			cfi->chips[i].buffer_write_time_max =
> +				1<<(cfi->cfiq->BufWriteTimeoutTyp +
> +					cfi->cfiq->BufWriteTimeoutMax);
> +			} else {
> +				cfi->chips[i].buffer_write_time_max = 0;
> +				}

Please keep the coding style as below.

		if ( ) {
			...
		} else {
			...
		}

Best regards,
Jingoo Han

> +		cfi->chips[i].buffer_write_time_max =
> +			((cfi->chips[i].buffer_write_time_max>= 2000)
> +			 ? cfi->chips[i].buffer_write_time_max : 2000);
>  		cfi->chips[i].ref_point_counter = 0;
>  		init_waitqueue_head(&(cfi->chips[i].wq));
>  	}
> @@ -1462,8 +1480,14 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
>  {
>  	struct cfi_private *cfi = map->fldrv_priv;
>  	unsigned long timeo = jiffies + HZ;
> -	/* see comments in do_write_oneword() regarding uWriteTimeo. */
> -	unsigned long uWriteTimeout = ( HZ / 1000 ) + 1;
> +	/*
> +	 * Use the result that calculated according to timeout field
> +	 * of struct cfi_ident that probed from norflash's CFI aera.
> +	 * See more comments in cfi_cmdset_0002().uWriteTimeout is
> +	 * used for timeout step,it must be concerted into jiffies.
> +	 */
> +	unsigned long uWriteTimeout =
> +				usecs_to_jiffies(chip->buffer_write_time_max);
>  	int ret = -EIO;
>  	unsigned long cmd_adr;
>  	int z, words;
> --
> 1.7.9.5 		 	   		  =

RE: [PATCH v5] mtd:nor:timeout:fix do_write_buffer() timeout error

[Bean Huo]

>> + if (cfi->cfiq->BufWriteTimeoutTyp &&
>> + cfi->cfiq->BufWriteTimeoutMax){
>> + cfi->chips[i].buffer_write_time_max =
>> + 1<<(cfi->cfiq->BufWriteTimeoutTyp +
>> + cfi->cfiq->BufWriteTimeoutMax);
>> + } else {
>> + cfi->chips[i].buffer_write_time_max = 0;
>> + }
>
> Please keep the coding style as below.
>
> if ( ) {
> ...
> } else {
> ...
> }
>

If I keep coding style as above.this will be beyond the requirements of one line length.
I also saw othter files use the same code style as mine.please see:cfi_cmdset_0001.c

if (cfi->cfiq->WordWriteTimeoutTyp &&
     cfi->cfiq->WordWriteTimeoutMax)
        cfi->chips[i].word_write_time_max =
		1<<(cfi->cfiq->WordWriteTimeoutTyp +
		      cfi->cfiq->WordWriteTimeoutMax);
else
      cfi->chips[i].word_write_time_max = 50000 * 8;

Note: beacuse I cann't send mail by git in office,I send this patch by web mail client.So,
there still a error stype in this patch(web mail client will remove some spaces),that is one space required before that '>='.I will send again this patch at home.
For your coding style advice, I will take into account,but i don't know if can put one line. 		 	   		  

Re: [PATCH v5] mtd:nor:timeout:fix do_write_buffer() timeout error

[Jingoo Han]

On Wednesday, July 16, 2014 5:49 PM, Bean Huo wrote:
> 
> >> + if (cfi->cfiq->BufWriteTimeoutTyp &&
> >> + cfi->cfiq->BufWriteTimeoutMax){
> >> + cfi->chips[i].buffer_write_time_max =
> >> + 1<<(cfi->cfiq->BufWriteTimeoutTyp +
> >> + cfi->cfiq->BufWriteTimeoutMax);
> >> + } else {
> >> + cfi->chips[i].buffer_write_time_max = 0;
> >> + }
> >
> > Please keep the coding style as below.
> >
> > if ( ) {
> > ...
> > } else {
> > ...
> > }
> >
> 
> If I keep coding style as above.this will be beyond the requirements of one line length.
> I also saw othter files use the same code style as mine.please see:cfi_cmdset_0001.c
> 
> if (cfi->cfiq->WordWriteTimeoutTyp &&
>      cfi->cfiq->WordWriteTimeoutMax)
>         cfi->chips[i].word_write_time_max =
> 		1<<(cfi->cfiq->WordWriteTimeoutTyp +
> 		      cfi->cfiq->WordWriteTimeoutMax);
> else
>       cfi->chips[i].word_write_time_max = 50000 * 8;

I don't want to mention about braces.
However, I said that you should keep the indentation.

Your original patch

+		if (cfi->cfiq->BufWriteTimeoutTyp &&
+			cfi->cfiq->BufWriteTimeoutMax){
+			cfi->chips[i].buffer_write_time_max =
+				1<<(cfi->cfiq->BufWriteTimeoutTyp +
+					cfi->cfiq->BufWriteTimeoutMax);
+			} else {
+				cfi->chips[i].buffer_write_time_max = 0;
+				}
+		cfi->chips[i].buffer_write_time_max =
+			((cfi->chips[i].buffer_write_time_max>= 2000)
+			 ? cfi->chips[i].buffer_write_time_max : 2000);

My suggestion is as follows: 

+		if (cfi->cfiq->BufWriteTimeoutTyp &&
+			cfi->cfiq->BufWriteTimeoutMax){
+			cfi->chips[i].buffer_write_time_max =
+				1<<(cfi->cfiq->BufWriteTimeoutTyp +
+					cfi->cfiq->BufWriteTimeoutMax);
+		} else {
+			cfi->chips[i].buffer_write_time_max = 0;
+		}
+		cfi->chips[i].buffer_write_time_max =
+			((cfi->chips[i].buffer_write_time_max>= 2000)
+			 ? cfi->chips[i].buffer_write_time_max : 2000);

Braces? I have no idea. Actually, I prefer to use braces for the
readability. However, according to the Document/CodingStyle, 
these braces are unnecessary.

However, one thing is clear; 'if' and 'else' should use the same
indentation level as below.

1. 
	if(){
		...
	} else {
		...
	}

2.
		if(){
			...
		} else {
			...
		}

3.

			if(){
				...
			} else {
				...
			}

Best regards,
Jingoo Han

> 
> Note: beacuse I cann't send mail by git in office,I send this patch by web mail client.So,
> there still a error stype in this patch(web mail client will remove some spaces),that is one space
> required before that '>='.I will send again this patch at home.
> For your coding style advice, I will take into account,but i don't know if can put one line.
> 	 	   		  =

RE: [PATCH v5] mtd:nor:timeout:fix do_write_buffer() timeout error

[Bean Huo]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 530 bytes --]

> However, one thing is clear; 'if' and 'else' should use the same
> indentation level as below.
>
> 1.
> if(){
> ...
> } else {
> ...
> }
>
> 2.
> if(){
> ...
> } else {
> ...
> }
>
> 3.
>
> if(){
> ...
> } else {
> ...
> }
>
> Best regards,
> Jingoo Han

hi,Jinngoo
I'm sorry for misunderstanding for you suggestion,and I will modify my patch in V6.thanks. 		 	   		  ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥