Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] Fix the HyperFlash support in the AMD/Fujitsu/Spansion CFI driver
@ 2019-10-03 18:29 Sergei Shtylyov
  2019-10-03 18:32 ` [PATCH 1/2] mtd: cfi_cmdset_0002: only check errors when ready in cfi_check_err_status() Sergei Shtylyov
  2019-10-03 18:34 ` [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash Sergei Shtylyov
  0 siblings, 2 replies; 9+ messages in thread
From: Sergei Shtylyov @ 2019-10-03 18:29 UTC (permalink / raw)
  To: David Woodhouse, Brian Norris, Marek Vasut, Miquel Raynal,
	Richard Weinberger, Vignesh Raghavendra, linux-mtd

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/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] mtd: cfi_cmdset_0002: only check errors when ready in cfi_check_err_status()
  2019-10-03 18:29 [PATCH 0/2] Fix the HyperFlash support in the AMD/Fujitsu/Spansion CFI driver Sergei Shtylyov
@ 2019-10-03 18:32 ` Sergei Shtylyov
  2019-10-03 18:34 ` [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash Sergei Shtylyov
  1 sibling, 0 replies; 9+ messages in thread
From: Sergei Shtylyov @ 2019-10-03 18:32 UTC (permalink / raw)
  To: David Woodhouse, Brian Norris, Marek Vasut, Miquel Raynal,
	Richard Weinberger, Vignesh Raghavendra, linux-mtd

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/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash
  2019-10-03 18:29 [PATCH 0/2] Fix the HyperFlash support in the AMD/Fujitsu/Spansion CFI driver Sergei Shtylyov
  2019-10-03 18:32 ` [PATCH 1/2] mtd: cfi_cmdset_0002: only check errors when ready in cfi_check_err_status() Sergei Shtylyov
@ 2019-10-03 18:34 ` Sergei Shtylyov
  2019-10-06 20:54   ` Tokunori Ikegami
  2019-10-16  6:33   ` Vignesh Raghavendra
  1 sibling, 2 replies; 9+ messages in thread
From: Sergei Shtylyov @ 2019-10-03 18:34 UTC (permalink / raw)
  To: David Woodhouse, Brian Norris, Marek Vasut, Miquel Raynal,
	Richard Weinberger, Vignesh Raghavendra, linux-mtd

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/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash
  2019-10-03 18:34 ` [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash Sergei Shtylyov
@ 2019-10-06 20:54   ` Tokunori Ikegami
  2019-10-11 12:03     ` Sergei Shtylyov
  2019-10-16  6:33   ` Vignesh Raghavendra
  1 sibling, 1 reply; 9+ messages in thread
From: Tokunori Ikegami @ 2019-10-06 20:54 UTC (permalink / raw)
  To: Sergei Shtylyov, David Woodhouse, Brian Norris, Marek Vasut,
	Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	linux-mtd

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/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash
  2019-10-06 20:54   ` Tokunori Ikegami
@ 2019-10-11 12:03     ` Sergei Shtylyov
  2019-10-13  3:35       ` Tokunori Ikegami
  0 siblings, 1 reply; 9+ messages in thread
From: Sergei Shtylyov @ 2019-10-11 12:03 UTC (permalink / raw)
  To: Tokunori Ikegami, David Woodhouse, Brian Norris, Marek Vasut,
	Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	linux-mtd

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/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash
  2019-10-11 12:03     ` Sergei Shtylyov
@ 2019-10-13  3:35       ` Tokunori Ikegami
  0 siblings, 0 replies; 9+ messages in thread
From: Tokunori Ikegami @ 2019-10-13  3:35 UTC (permalink / raw)
  To: Sergei Shtylyov, David Woodhouse, Brian Norris, Marek Vasut,
	Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	linux-mtd

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/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash
  2019-10-03 18:34 ` [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash Sergei Shtylyov
  2019-10-06 20:54   ` Tokunori Ikegami
@ 2019-10-16  6:33   ` Vignesh Raghavendra
  2019-10-28 19:15     ` Sergei Shtylyov
  1 sibling, 1 reply; 9+ messages in thread
From: Vignesh Raghavendra @ 2019-10-16  6:33 UTC (permalink / raw)
  To: Sergei Shtylyov, David Woodhouse, Brian Norris, Marek Vasut,
	Miquel Raynal, Richard Weinberger, linux-mtd

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/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash
  2019-10-16  6:33   ` Vignesh Raghavendra
@ 2019-10-28 19:15     ` Sergei Shtylyov
  2019-10-30 15:35       ` Vignesh Raghavendra
  0 siblings, 1 reply; 9+ messages in thread
From: Sergei Shtylyov @ 2019-10-28 19:15 UTC (permalink / raw)
  To: Vignesh Raghavendra, David Woodhouse, Brian Norris, Marek Vasut,
	Miquel Raynal, Richard Weinberger, linux-mtd

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/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash
  2019-10-28 19:15     ` Sergei Shtylyov
@ 2019-10-30 15:35       ` Vignesh Raghavendra
  0 siblings, 0 replies; 9+ messages in thread
From: Vignesh Raghavendra @ 2019-10-30 15:35 UTC (permalink / raw)
  To: Sergei Shtylyov, David Woodhouse, Brian Norris, Marek Vasut,
	Miquel Raynal, Richard Weinberger, linux-mtd

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/

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-03 18:29 [PATCH 0/2] Fix the HyperFlash support in the AMD/Fujitsu/Spansion CFI driver Sergei Shtylyov
2019-10-03 18:32 ` [PATCH 1/2] mtd: cfi_cmdset_0002: only check errors when ready in cfi_check_err_status() Sergei Shtylyov
2019-10-03 18:34 ` [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash Sergei Shtylyov
2019-10-06 20:54   ` Tokunori Ikegami
2019-10-11 12:03     ` Sergei Shtylyov
2019-10-13  3:35       ` Tokunori Ikegami
2019-10-16  6:33   ` Vignesh Raghavendra
2019-10-28 19:15     ` Sergei Shtylyov
2019-10-30 15:35       ` Vignesh Raghavendra

Linux-mtd Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mtd/0 linux-mtd/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mtd linux-mtd/ https://lore.kernel.org/linux-mtd \
		linux-mtd@lists.infradead.org
	public-inbox-index linux-mtd

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mtd


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git