[PATCH 0/2] Fix the HyperFlash support in the AMD/Fujitsu/Spansion CFI driver

[PATCH 0/2] Fix the HyperFlash support in the AMD/Fujitsu/Spansion CFI driver

[Sergei Shtylyov]

Hello!

Here's a set of 2 patches against the current Linus' tree (the 'mtd/fixes'
branch of the MTD repo is outdated).
Linux 5.4-rc1 received initial support for the HyperFlash. However, the patch
adding the status register polling to the AMD/Fujitsu/Spansion CFI driver has
couple issues which I'm trying to solve here...

[1/2] mtd: cfi_cmdset_0002: only check errors when ready in cfi_check_err_status()
[2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash

MBR, Sergei

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

[PATCH 1/2] mtd: cfi_cmdset_0002: only check errors when ready in cfi_check_err_status()

[Sergei Shtylyov]

Cypress S26K{L|S}P{128|256|512}S datasheet says that the error bits in
the status register are only valid when the "device ready" bit 7 is set.
Add the check for the device ready bit in cfi_check_err_status() as that
function isn't always called with this bit set.

Fixes: 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling status register")
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/mtd/chips/cfi_cmdset_0002.c |    4 ++++
 1 file changed, 4 insertions(+)

Index: linux/drivers/mtd/chips/cfi_cmdset_0002.c
===================================================================
--- linux.orig/drivers/mtd/chips/cfi_cmdset_0002.c
+++ linux/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -136,6 +136,10 @@ static void cfi_check_err_status(struct
 			 cfi->device_type, NULL);
 	status = map_read(map, adr);
 
+	/* The error bits are invalid while the chip's busy */
+	if (!map_word_bitsset(map, status, CMD(CFI_SR_DRB)))
+		return;
+
 	if (map_word_bitsset(map, status, CMD(0x3a))) {
 		unsigned long chipstatus = MERGESTATUS(status);
 

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

[PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash

[Sergei Shtylyov]

The commit 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling
status register") added checking for the status register error bits into
chip_good() to only return 1 if these bits are zero. Unfortunately, this
means that polling using chip_good() always reaches a time-out condition
when erase or program failure bits are set. I think the status register
error checking should be fully delegated to cfi_check_err_status() that
should return whether any error bits were set or not...

Fixes: 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling status register")
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/mtd/chips/cfi_cmdset_0002.c |   55 +++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 25 deletions(-)

Index: linux/drivers/mtd/chips/cfi_cmdset_0002.c
===================================================================
--- linux.orig/drivers/mtd/chips/cfi_cmdset_0002.c
+++ linux/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -123,14 +123,14 @@ static int cfi_use_status_reg(struct cfi
 		(extp->SoftwareFeatures & poll_mask) == CFI_POLL_STATUS_REG;
 }
 
-static void cfi_check_err_status(struct map_info *map, struct flchip *chip,
-				 unsigned long adr)
+static int cfi_check_err_status(struct map_info *map, struct flchip *chip,
+				unsigned long adr)
 {
 	struct cfi_private *cfi = map->fldrv_priv;
 	map_word status;
 
 	if (!cfi_use_status_reg(cfi))
-		return;
+		return 0;
 
 	cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi,
 			 cfi->device_type, NULL);
@@ -138,7 +138,7 @@ static void cfi_check_err_status(struct
 
 	/* The error bits are invalid while the chip's busy */
 	if (!map_word_bitsset(map, status, CMD(CFI_SR_DRB)))
-		return;
+		return 0;
 
 	if (map_word_bitsset(map, status, CMD(0x3a))) {
 		unsigned long chipstatus = MERGESTATUS(status);
@@ -155,7 +155,9 @@ static void cfi_check_err_status(struct
 		if (chipstatus & CFI_SR_SLSB)
 			pr_err("%s sector write protected, status %lx\n",
 			       map->name, chipstatus);
+		return 1;
 	}
+	return 0;
 }
 
 /* #define DEBUG_CFI_FEATURES */
@@ -852,20 +854,16 @@ static int __xipram chip_good(struct map
 
 	if (cfi_use_status_reg(cfi)) {
 		map_word ready = CMD(CFI_SR_DRB);
-		map_word err = CMD(CFI_SR_PSB | CFI_SR_ESB);
+
 		/*
 		 * For chips that support status register, check device
-		 * ready bit and Erase/Program status bit to know if
-		 * operation succeeded.
+		 * ready bit
 		 */
 		cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi,
 				 cfi->device_type, NULL);
 		curd = map_read(map, addr);
 
-		if (map_word_andequal(map, curd, ready, ready))
-			return !map_word_bitsset(map, curd, err);
-
-		return 0;
+		return map_word_andequal(map, curd, ready, ready);
 	}
 
 	oldd = map_read(map, addr);
@@ -1703,8 +1701,11 @@ static int __xipram do_write_oneword_onc
 			break;
 		}
 
-		if (chip_good(map, chip, adr, datum))
+		if (chip_good(map, chip, adr, datum)) {
+			if (cfi_check_err_status(map, chip, adr))
+				ret = -EIO;
 			break;
+		}
 
 		/* Latency issues. Drop the lock, wait a while and retry */
 		UDELAY(map, chip, adr, 1);
@@ -1777,7 +1778,6 @@ static int __xipram do_write_oneword_ret
 	ret = do_write_oneword_once(map, chip, adr, datum, mode, cfi);
 	if (ret) {
 		/* reset on all failures. */
-		cfi_check_err_status(map, chip, adr);
 		map_write(map, CMD(0xF0), chip->start);
 		/* FIXME - should have reset delay before continuing */
 
@@ -1974,12 +1974,17 @@ static int __xipram do_write_buffer_wait
 		 */
 		if (time_after(jiffies, timeo) &&
 		    !chip_good(map, chip, adr, datum)) {
+			pr_warn("MTD %s(): software timeout, address:0x%.8lx.\n",
+				__func__, adr);
 			ret = -EIO;
 			break;
 		}
 
-		if (chip_good(map, chip, adr, datum))
+		if (chip_good(map, chip, adr, datum)) {
+			if (cfi_check_err_status(map, chip, adr))
+				ret = -EIO;
 			break;
+		}
 
 		/* Latency issues. Drop the lock, wait a while and retry */
 		UDELAY(map, chip, adr, 1);
@@ -2075,12 +2080,8 @@ static int __xipram do_write_buffer(stru
 				chip->word_write_time);
 
 	ret = do_write_buffer_wait(map, chip, adr, datum);
-	if (ret) {
-		cfi_check_err_status(map, chip, adr);
+	if (ret)
 		do_write_buffer_reset(map, chip, cfi);
-		pr_err("MTD %s(): software timeout, address:0x%.8lx.\n",
-		       __func__, adr);
-	}
 
 	xip_enable(map, chip, adr);
 
@@ -2275,9 +2276,9 @@ retry:
 		udelay(1);
 	}
 
-	if (!chip_good(map, chip, adr, datum)) {
+	if (!chip_good(map, chip, adr, datum) ||
+	    cfi_check_err_status(map, chip, adr)) {
 		/* reset on all failures. */
-		cfi_check_err_status(map, chip, adr);
 		map_write(map, CMD(0xF0), chip->start);
 		/* FIXME - should have reset delay before continuing */
 
@@ -2471,8 +2472,11 @@ static int __xipram do_erase_chip(struct
 			chip->erase_suspended = 0;
 		}
 
-		if (chip_good(map, chip, adr, map_word_ff(map)))
+		if (chip_good(map, chip, adr, map_word_ff(map))) {
+			if (cfi_check_err_status(map, chip, adr))
+				ret = -EIO;
 			break;
+		}
 
 		if (time_after(jiffies, timeo)) {
 			printk(KERN_WARNING "MTD %s(): software timeout\n",
@@ -2487,7 +2491,6 @@ static int __xipram do_erase_chip(struct
 	/* Did we succeed? */
 	if (ret) {
 		/* reset on all failures. */
-		cfi_check_err_status(map, chip, adr);
 		map_write(map, CMD(0xF0), chip->start);
 		/* FIXME - should have reset delay before continuing */
 
@@ -2568,8 +2571,11 @@ static int __xipram do_erase_oneblock(st
 			chip->erase_suspended = 0;
 		}
 
-		if (chip_good(map, chip, adr, map_word_ff(map)))
+		if (chip_good(map, chip, adr, map_word_ff(map))) {
+			if (cfi_check_err_status(map, chip, adr))
+				ret = -EIO;
 			break;
+		}
 
 		if (time_after(jiffies, timeo)) {
 			printk(KERN_WARNING "MTD %s(): software timeout\n",
@@ -2584,7 +2590,6 @@ static int __xipram do_erase_oneblock(st
 	/* Did we succeed? */
 	if (ret) {
 		/* reset on all failures. */
-		cfi_check_err_status(map, chip, adr);
 		map_write(map, CMD(0xF0), chip->start);
 		/* FIXME - should have reset delay before continuing */
 

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

Re: [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash

[Tokunori Ikegami]

Hi,

On 2019/10/04 3:34, Sergei Shtylyov wrote:
> The commit 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling
> status register") added checking for the status register error bits into
> chip_good() to only return 1 if these bits are zero. Unfortunately, this
> means that polling using chip_good() always reaches a time-out condition
> when erase or program failure bits are set. I think the status register
> error checking should be fully delegated to cfi_check_err_status() that
> should return whether any error bits were set or not...
>
> Fixes: 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling status register")
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>
> ---
>   drivers/mtd/chips/cfi_cmdset_0002.c |   55 +++++++++++++++++++-----------------
>   1 file changed, 30 insertions(+), 25 deletions(-)
>
> Index: linux/drivers/mtd/chips/cfi_cmdset_0002.c
> ===================================================================
> --- linux.orig/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ linux/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -123,14 +123,14 @@ static int cfi_use_status_reg(struct cfi
>   		(extp->SoftwareFeatures & poll_mask) == CFI_POLL_STATUS_REG;
>   }
>   
> -static void cfi_check_err_status(struct map_info *map, struct flchip *chip,
> -				 unsigned long adr)
> +static int cfi_check_err_status(struct map_info *map, struct flchip *chip,
> +				unsigned long adr)
>   {
>   	struct cfi_private *cfi = map->fldrv_priv;
>   	map_word status;
>   
>   	if (!cfi_use_status_reg(cfi))
> -		return;
> +		return 0;
>   
>   	cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi,
>   			 cfi->device_type, NULL);
> @@ -138,7 +138,7 @@ static void cfi_check_err_status(struct
>   
>   	/* The error bits are invalid while the chip's busy */
>   	if (!map_word_bitsset(map, status, CMD(CFI_SR_DRB)))
> -		return;
> +		return 0;
>   
>   	if (map_word_bitsset(map, status, CMD(0x3a))) {
>   		unsigned long chipstatus = MERGESTATUS(status);
> @@ -155,7 +155,9 @@ static void cfi_check_err_status(struct
>   		if (chipstatus & CFI_SR_SLSB)
>   			pr_err("%s sector write protected, status %lx\n",
>   			       map->name, chipstatus);
> +		return 1;
Is it okay to be returned 1 for the errors CFI_SR_WBASB and CFI_SR_SLSB 
also?
Before the change only CFI_SR_ESB and CFI_SR_PSB were checked by 
chip_good().
>   	}
> +	return 0;
>   }
>   
>   /* #define DEBUG_CFI_FEATURES */
> @@ -852,20 +854,16 @@ static int __xipram chip_good(struct map
>   
>   	if (cfi_use_status_reg(cfi)) {
>   		map_word ready = CMD(CFI_SR_DRB);
> -		map_word err = CMD(CFI_SR_PSB | CFI_SR_ESB);
> +
>   		/*
>   		 * For chips that support status register, check device
> -		 * ready bit and Erase/Program status bit to know if
> -		 * operation succeeded.
> +		 * ready bit
>   		 */
>   		cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi,
>   				 cfi->device_type, NULL);
>   		curd = map_read(map, addr);
>   
> -		if (map_word_andequal(map, curd, ready, ready))
> -			return !map_word_bitsset(map, curd, err);
> -
> -		return 0;
> +		return map_word_andequal(map, curd, ready, ready);
>   	}
>   
>   	oldd = map_read(map, addr);
> @@ -1703,8 +1701,11 @@ static int __xipram do_write_oneword_onc
>   			break;
>   		}
>   
> -		if (chip_good(map, chip, adr, datum))
> +		if (chip_good(map, chip, adr, datum)) {
> +			if (cfi_check_err_status(map, chip, adr))
> +				ret = -EIO;
>   			break;
> +		}
>   
>   		/* Latency issues. Drop the lock, wait a while and retry */
>   		UDELAY(map, chip, adr, 1);
> @@ -1777,7 +1778,6 @@ static int __xipram do_write_oneword_ret
>   	ret = do_write_oneword_once(map, chip, adr, datum, mode, cfi);
>   	if (ret) {
>   		/* reset on all failures. */
> -		cfi_check_err_status(map, chip, adr);
>   		map_write(map, CMD(0xF0), chip->start);
>   		/* FIXME - should have reset delay before continuing */
>   
> @@ -1974,12 +1974,17 @@ static int __xipram do_write_buffer_wait
>   		 */
>   		if (time_after(jiffies, timeo) &&
>   		    !chip_good(map, chip, adr, datum)) {
> +			pr_warn("MTD %s(): software timeout, address:0x%.8lx.\n",
> +				__func__, adr);

Is there any reason to change the message to use pr_warn() from pr_err()?
In the past it was changed to use pr_err() from printk(KERN_WARNING) as 
mentioned by the maintainer.

Regards,
Ikegami

>   			ret = -EIO;
>   			break;
>   		}
>   
> -		if (chip_good(map, chip, adr, datum))
> +		if (chip_good(map, chip, adr, datum)) {
> +			if (cfi_check_err_status(map, chip, adr))
> +				ret = -EIO;
>   			break;
> +		}
>   
>   		/* Latency issues. Drop the lock, wait a while and retry */
>   		UDELAY(map, chip, adr, 1);
> @@ -2075,12 +2080,8 @@ static int __xipram do_write_buffer(stru
>   				chip->word_write_time);
>   
>   	ret = do_write_buffer_wait(map, chip, adr, datum);
> -	if (ret) {
> -		cfi_check_err_status(map, chip, adr);
> +	if (ret)
>   		do_write_buffer_reset(map, chip, cfi);
> -		pr_err("MTD %s(): software timeout, address:0x%.8lx.\n",
> -		       __func__, adr);
> -	}
>   
>   	xip_enable(map, chip, adr);
>   
> @@ -2275,9 +2276,9 @@ retry:
>   		udelay(1);
>   	}
>   
> -	if (!chip_good(map, chip, adr, datum)) {
> +	if (!chip_good(map, chip, adr, datum) ||
> +	    cfi_check_err_status(map, chip, adr)) {
>   		/* reset on all failures. */
> -		cfi_check_err_status(map, chip, adr);
>   		map_write(map, CMD(0xF0), chip->start);
>   		/* FIXME - should have reset delay before continuing */
>   
> @@ -2471,8 +2472,11 @@ static int __xipram do_erase_chip(struct
>   			chip->erase_suspended = 0;
>   		}
>   
> -		if (chip_good(map, chip, adr, map_word_ff(map)))
> +		if (chip_good(map, chip, adr, map_word_ff(map))) {
> +			if (cfi_check_err_status(map, chip, adr))
> +				ret = -EIO;
>   			break;
> +		}
>   
>   		if (time_after(jiffies, timeo)) {
>   			printk(KERN_WARNING "MTD %s(): software timeout\n",
> @@ -2487,7 +2491,6 @@ static int __xipram do_erase_chip(struct
>   	/* Did we succeed? */
>   	if (ret) {
>   		/* reset on all failures. */
> -		cfi_check_err_status(map, chip, adr);
>   		map_write(map, CMD(0xF0), chip->start);
>   		/* FIXME - should have reset delay before continuing */
>   
> @@ -2568,8 +2571,11 @@ static int __xipram do_erase_oneblock(st
>   			chip->erase_suspended = 0;
>   		}
>   
> -		if (chip_good(map, chip, adr, map_word_ff(map)))
> +		if (chip_good(map, chip, adr, map_word_ff(map))) {
> +			if (cfi_check_err_status(map, chip, adr))
> +				ret = -EIO;
>   			break;
> +		}
>   
>   		if (time_after(jiffies, timeo)) {
>   			printk(KERN_WARNING "MTD %s(): software timeout\n",
> @@ -2584,7 +2590,6 @@ static int __xipram do_erase_oneblock(st
>   	/* Did we succeed? */
>   	if (ret) {
>   		/* reset on all failures. */
> -		cfi_check_err_status(map, chip, adr);
>   		map_write(map, CMD(0xF0), chip->start);
>   		/* FIXME - should have reset delay before continuing */
>   
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>

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

Re: [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash

[Sergei Shtylyov]

On 10/06/2019 11:54 PM, Tokunori Ikegami wrote:

>> The commit 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling
>> status register") added checking for the status register error bits into
>> chip_good() to only return 1 if these bits are zero. Unfortunately, this
>> means that polling using chip_good() always reaches a time-out condition
>> when erase or program failure bits are set. I think the status register
>> error checking should be fully delegated to cfi_check_err_status() that
>> should return whether any error bits were set or not...
>>
>> Fixes: 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling status register")
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>
>> ---
>>   drivers/mtd/chips/cfi_cmdset_0002.c |   55 +++++++++++++++++++-----------------
>>   1 file changed, 30 insertions(+), 25 deletions(-)
>>
>> Index: linux/drivers/mtd/chips/cfi_cmdset_0002.c
>> ===================================================================
>> --- linux.orig/drivers/mtd/chips/cfi_cmdset_0002.c
>> +++ linux/drivers/mtd/chips/cfi_cmdset_0002.c
[...]
>> @@ -155,7 +155,9 @@ static void cfi_check_err_status(struct
>>           if (chipstatus & CFI_SR_SLSB)
>>               pr_err("%s sector write protected, status %lx\n",
>>                      map->name, chipstatus);
>> +        return 1;
> Is it okay to be returned 1 for the errors CFI_SR_WBASB and CFI_SR_SLSB also?
> Before the change only CFI_Well, SR_ESB and CFI_SR_PSB were checked by chip_good().

   Well, pr_err() calls above spoke for themselves: all bitmask 0x3a was considered
the error bits. But I can change that back to just ESB/PSB if preferred.

>>       }
>> +    return 0;
>>   }
>>     /* #define DEBUG_CFI_FEATURES */
[...]
>>   @@ -1974,12 +1974,17 @@ static int __xipram do_write_buffer_wait
>>            */
>>           if (time_after(jiffies, timeo) &&
>>               !chip_good(map, chip, adr, datum)) {
>> +            pr_warn("MTD %s(): software timeout, address:0x%.8lx.\n",
>> +                __func__, adr);
> 
> Is there any reason to change the message to use pr_warn() from pr_err()?

   Yes, all the other timeout messages use printk(KERN_WARNING, ...);

> In the past it was changed to use pr_err() from printk(KERN_WARNING) as mentioned by the maintainer.

   Oh, OK, I'll switch back if so...

> 
> Regards,
> Ikegami
[...]

   It's better to remove the diff you're not commenting to.

MBR, Sergei

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

Re: [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash

[Tokunori Ikegami]

Hi,

>>>            if (chipstatus & CFI_SR_SLSB)
>>>                pr_err("%s sector write protected, status %lx\n",
>>>                       map->name, chipstatus);
>>> +        return 1;
>> Is it okay to be returned 1 for the errors CFI_SR_WBASB and CFI_SR_SLSB also?
>> Before the change only CFI_Well, SR_ESB and CFI_SR_PSB were checked by chip_good().
>     Well, pr_err() calls above spoke for themselves: all bitmask 0x3a was considered
> the error bits. But I can change that back to just ESB/PSB if preferred.
It was mentioned before as it is enough to see if ESB or PSB is set to 
know if write or erase failed.
>     It's better to remove the diff you're not commenting to.

Noted it.

Regards,
Ikegami


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

Re: [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash

[Vignesh Raghavendra]

Hi,

On 04/10/19 12:04 AM, Sergei Shtylyov wrote:
> The commit 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling
> status register") added checking for the status register error bits into
> chip_good() to only return 1 if these bits are zero. Unfortunately, this
> means that polling using chip_good() always reaches a time-out condition
> when erase or program failure bits are set. I think the status register
> error checking should be fully delegated to cfi_check_err_status() that
> should return whether any error bits were set or not...
> 

Please reword last sentence to drop "I think". Something like:

Lets fully delegate the function of determining error condition to
cfi_check_err_status() and make chip_good() only look for Device
Ready/Busy condition.

> Fixes: 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling status register")
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
>  drivers/mtd/chips/cfi_cmdset_0002.c |   55 +++++++++++++++++++-----------------
>  1 file changed, 30 insertions(+), 25 deletions(-)
> 
> Index: linux/drivers/mtd/chips/cfi_cmdset_0002.c
> ===================================================================
> --- linux.orig/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ linux/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -123,14 +123,14 @@ static int cfi_use_status_reg(struct cfi
>  		(extp->SoftwareFeatures & poll_mask) == CFI_POLL_STATUS_REG;
>  }
>  
> -static void cfi_check_err_status(struct map_info *map, struct flchip *chip,
> -				 unsigned long adr)
> +static int cfi_check_err_status(struct map_info *map, struct flchip *chip,
> +				unsigned long adr)
>  {
>  	struct cfi_private *cfi = map->fldrv_priv;
>  	map_word status;
>  
>  	if (!cfi_use_status_reg(cfi))
> -		return;
> +		return 0;
>  
>  	cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi,
>  			 cfi->device_type, NULL);
> @@ -138,7 +138,7 @@ static void cfi_check_err_status(struct
>  
>  	/* The error bits are invalid while the chip's busy */
>  	if (!map_word_bitsset(map, status, CMD(CFI_SR_DRB)))
> -		return;
> +		return 0;
>  
>  	if (map_word_bitsset(map, status, CMD(0x3a))) {
>  		unsigned long chipstatus = MERGESTATUS(status);
> @@ -155,7 +155,9 @@ static void cfi_check_err_status(struct
>  		if (chipstatus & CFI_SR_SLSB)
>  			pr_err("%s sector write protected, status %lx\n",
>  			       map->name, chipstatus);
> +		return 1;
>  	}
> +	return 0;
>  }
>  
>  /* #define DEBUG_CFI_FEATURES */
> @@ -852,20 +854,16 @@ static int __xipram chip_good(struct map
>  
>  	if (cfi_use_status_reg(cfi)) {
>  		map_word ready = CMD(CFI_SR_DRB);
> -		map_word err = CMD(CFI_SR_PSB | CFI_SR_ESB);
> +
>  		/*
>  		 * For chips that support status register, check device
> -		 * ready bit and Erase/Program status bit to know if
> -		 * operation succeeded.
> +		 * ready bit
>  		 */
>  		cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi,
>  				 cfi->device_type, NULL);
>  		curd = map_read(map, addr);
>  
> -		if (map_word_andequal(map, curd, ready, ready))
> -			return !map_word_bitsset(map, curd, err);
> -
> -		return 0;
> +		return map_word_andequal(map, curd, ready, ready);
>  	}
>  
>  	oldd = map_read(map, addr);
> @@ -1703,8 +1701,11 @@ static int __xipram do_write_oneword_onc

Nit: for some reason, your diff has function names truncated abruptly
which makes its slightly harder to locate the context. I use git
format-patch that produces better readable contexts.

>  			break;
>  		}
>  
> -		if (chip_good(map, chip, adr, datum))
> +		if (chip_good(map, chip, adr, datum)) {
> +			if (cfi_check_err_status(map, chip, adr))
> +				ret = -EIO;
>  			break;
> +		}
>  
>  		/* Latency issues. Drop the lock, wait a while and retry */
>  		UDELAY(map, chip, adr, 1);
> @@ -1777,7 +1778,6 @@ static int __xipram do_write_oneword_ret
>  	ret = do_write_oneword_once(map, chip, adr, datum, mode, cfi);
>  	if (ret) {
>  		/* reset on all failures. */
> -		cfi_check_err_status(map, chip, adr);
>  		map_write(map, CMD(0xF0), chip->start);
>  		/* FIXME - should have reset delay before continuing */
>  
> @@ -1974,12 +1974,17 @@ static int __xipram do_write_buffer_wait
>  		 */
>  		if (time_after(jiffies, timeo) &&
>  		    !chip_good(map, chip, adr, datum)) {
> +			pr_warn("MTD %s(): software timeout, address:0x%.8lx.\n",
> +				__func__, adr);

Since we are returning an error condition, this should be pr_err() (I
know that rest of the file does not follow this convention, but lets
make sure new code does)


Rest looks fine to me. Thanks for the patch!

-- 
Regards
Vignesh

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

Re: [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash

[Sergei Shtylyov]

Hello!

On 10/16/2019 09:33 AM, Vignesh Raghavendra wrote:

>> The commit 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling
>> status register") added checking for the status register error bits into
>> chip_good() to only return 1 if these bits are zero. Unfortunately, this
>> means that polling using chip_good() always reaches a time-out condition
>> when erase or program failure bits are set. I think the status register
>> error checking should be fully delegated to cfi_check_err_status() that
>> should return whether any error bits were set or not...
>>
> 
> Please reword last sentence to drop "I think". Something like:
> 
> Lets fully delegate the function of determining error condition to
> cfi_check_err_status() and make chip_good() only look for Device
> Ready/Busy condition.

   OK. :-)

>> Fixes: 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling status register")
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>
>> ---
>>  drivers/mtd/chips/cfi_cmdset_0002.c |   55 +++++++++++++++++++-----------------
>>  1 file changed, 30 insertions(+), 25 deletions(-)
>>
>> Index: linux/drivers/mtd/chips/cfi_cmdset_0002.c
>> ===================================================================
>> --- linux.orig/drivers/mtd/chips/cfi_cmdset_0002.c
>> +++ linux/drivers/mtd/chips/cfi_cmdset_0002.c
>> @@ -123,14 +123,14 @@ static int cfi_use_status_reg(struct cfi
>>  		(extp->SoftwareFeatures & poll_mask) == CFI_POLL_STATUS_REG;
>>  }
>>  
>> -static void cfi_check_err_status(struct map_info *map, struct flchip *chip,
>> -				 unsigned long adr)
>> +static int cfi_check_err_status(struct map_info *map, struct flchip *chip,
>> +				unsigned long adr)
>>  {
>>  	struct cfi_private *cfi = map->fldrv_priv;
>>  	map_word status;
>>  
>>  	if (!cfi_use_status_reg(cfi))
>> -		return;
>> +		return 0;
>>  
>>  	cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi,
>>  			 cfi->device_type, NULL);
>> @@ -138,7 +138,7 @@ static void cfi_check_err_status(struct
>>  
>>  	/* The error bits are invalid while the chip's busy */
>>  	if (!map_word_bitsset(map, status, CMD(CFI_SR_DRB)))
>> -		return;
>> +		return 0;
>>  
>>  	if (map_word_bitsset(map, status, CMD(0x3a))) {
>>  		unsigned long chipstatus = MERGESTATUS(status);
>> @@ -155,7 +155,9 @@ static void cfi_check_err_status(struct
>>  		if (chipstatus & CFI_SR_SLSB)
>>  			pr_err("%s sector write protected, status %lx\n",
>>  			       map->name, chipstatus);
>> +		return 1;

   So are you OK with extending the set of the error signalling bits I
did here, or I should really have accounted only for ESB and PSB bits
being error signals?

>>  	}
>> +	return 0;
>>  }
>>  
>>  /* #define DEBUG_CFI_FEATURES */
[...]
>> @@ -1703,8 +1701,11 @@ static int __xipram do_write_oneword_onc
> 
> Nit: for some reason, your diff has function names truncated abruptly
> which makes its slightly harder to locate the context. I use git
> format-patch that produces better readable contexts.

   I use quilt for development, not a big fan of git in this role. :-)

[...]
>> @@ -1974,12 +1974,17 @@ static int __xipram do_write_buffer_wait
>>  		 */
>>  		if (time_after(jiffies, timeo) &&
>>  		    !chip_good(map, chip, adr, datum)) {
>> +			pr_warn("MTD %s(): software timeout, address:0x%.8lx.\n",
>> +				__func__, adr);
> 
> Since we are returning an error condition, this should be pr_err() (I
> know that rest of the file does not follow this convention, but lets
> make sure new code does)

   OK, I was looking at the other timeout code and failed to notice
that this printk() was converted to pr_err() by Ikegami-san... 

> Rest looks fine to me. Thanks for the patch!

   TY for the review.

MBR, Sergei

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

Re: [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash

[Vignesh Raghavendra]

Hi,

On 28/10/19 8:15 PM, Sergei Shtylyov wrote:
> Hello!
> 
> On 10/16/2019 09:33 AM, Vignesh Raghavendra wrote:
> 
>>> The commit 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling
>>> status register") added checking for the status register error bits into
>>> chip_good() to only return 1 if these bits are zero. Unfortunately, this
>>> means that polling using chip_good() always reaches a time-out condition
>>> when erase or program failure bits are set. I think the status register
>>> error checking should be fully delegated to cfi_check_err_status() that
>>> should return whether any error bits were set or not...
>>>
>>
>> Please reword last sentence to drop "I think". Something like:
>>
>> Lets fully delegate the function of determining error condition to
>> cfi_check_err_status() and make chip_good() only look for Device
>> Ready/Busy condition.
> 
>    OK. :-)
> 
>>> Fixes: 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling status register")
>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>>
>>> ---
>>>  drivers/mtd/chips/cfi_cmdset_0002.c |   55 +++++++++++++++++++-----------------
>>>  1 file changed, 30 insertions(+), 25 deletions(-)
>>>
>>> Index: linux/drivers/mtd/chips/cfi_cmdset_0002.c
>>> ===================================================================
>>> --- linux.orig/drivers/mtd/chips/cfi_cmdset_0002.c
>>> +++ linux/drivers/mtd/chips/cfi_cmdset_0002.c
>>> @@ -123,14 +123,14 @@ static int cfi_use_status_reg(struct cfi
>>>  		(extp->SoftwareFeatures & poll_mask) == CFI_POLL_STATUS_REG;
>>>  }
>>>  
>>> -static void cfi_check_err_status(struct map_info *map, struct flchip *chip,
>>> -				 unsigned long adr)
>>> +static int cfi_check_err_status(struct map_info *map, struct flchip *chip,
>>> +				unsigned long adr)
>>>  {
>>>  	struct cfi_private *cfi = map->fldrv_priv;
>>>  	map_word status;
>>>  
>>>  	if (!cfi_use_status_reg(cfi))
>>> -		return;
>>> +		return 0;
>>>  
>>>  	cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi,
>>>  			 cfi->device_type, NULL);
>>> @@ -138,7 +138,7 @@ static void cfi_check_err_status(struct
>>>  
>>>  	/* The error bits are invalid while the chip's busy */
>>>  	if (!map_word_bitsset(map, status, CMD(CFI_SR_DRB)))
>>> -		return;
>>> +		return 0;
>>>  
>>>  	if (map_word_bitsset(map, status, CMD(0x3a))) {
>>>  		unsigned long chipstatus = MERGESTATUS(status);
>>> @@ -155,7 +155,9 @@ static void cfi_check_err_status(struct
>>>  		if (chipstatus & CFI_SR_SLSB)
>>>  			pr_err("%s sector write protected, status %lx\n",
>>>  			       map->name, chipstatus);
>>> +		return 1;
> 
>    So are you OK with extending the set of the error signalling bits I
> did here, or I should really have accounted only for ESB and PSB bits
> being error signals?
> 

I am fine either way. Because as per HyperFlash datasheet, PSB or ESB
will always be set if either of WASB or SLSB is set. So it does not
really matter if we are checking for just ESB and PSB or otherwise.

Regards
Vignesh

>>>  	}
>>> +	return 0;
>>>  }
>>>  
>>>  /* #define DEBUG_CFI_FEATURES */
> [...]
>>> @@ -1703,8 +1701,11 @@ static int __xipram do_write_oneword_onc
>>
>> Nit: for some reason, your diff has function names truncated abruptly
>> which makes its slightly harder to locate the context. I use git
>> format-patch that produces better readable contexts.
> 
>    I use quilt for development, not a big fan of git in this role. :-)
> 
> [...]
>>> @@ -1974,12 +1974,17 @@ static int __xipram do_write_buffer_wait
>>>  		 */
>>>  		if (time_after(jiffies, timeo) &&
>>>  		    !chip_good(map, chip, adr, datum)) {
>>> +			pr_warn("MTD %s(): software timeout, address:0x%.8lx.\n",
>>> +				__func__, adr);
>>
>> Since we are returning an error condition, this should be pr_err() (I
>> know that rest of the file does not follow this convention, but lets
>> make sure new code does)
> 
>    OK, I was looking at the other timeout code and failed to notice
> that this printk() was converted to pr_err() by Ikegami-san... 
> 
>> Rest looks fine to me. Thanks for the patch!
> 
>    TY for the review.
> 
> MBR, Sergei
> 

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