[PATCH] Bug in m25p80.c during whole-chip erase

[PATCH] Bug in m25p80.c during whole-chip erase

[Steven A. Falco]

There is a logic error in "whole chip erase" for the m25p80 family.  If
the whole device is successfully erased, erase_chip() will return 0, and
the code will fall through to the "else" clause, and do sector-by-sector
erase in addition to the whole-chip erase.  This patch corrects that.

Also, the MAX_READY_WAIT_COUNT is insufficient for an m25p16 connected
to a 400 MHz powerpc.  Increasing it allows me to successfully program
the device on my board.

Signed-off-by: Steven A. Falco <sfalco@harris.com>
---

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 6659b22..3a2fed8 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -53,7 +53,7 @@
 #define	SR_SRWD			0x80	/* SR write protect */
 
 /* Define max times to check status register before we give up. */
-#define	MAX_READY_WAIT_COUNT	100000
+#define	MAX_READY_WAIT_COUNT	1000000
 #define	CMD_SIZE		4
 
 #ifdef CONFIG_M25PXX_USE_FAST_READ
@@ -251,10 +251,12 @@ static int m25p80_erase(struct mtd_info *mtd, struct erase_info *instr)
 	mutex_lock(&flash->lock);
 
 	/* whole-chip erase? */
-	if (len == flash->mtd.size && erase_chip(flash)) {
-		instr->state = MTD_ERASE_FAILED;
-		mutex_unlock(&flash->lock);
-		return -EIO;
+	if (len == flash->mtd.size) {
+		if (erase_chip(flash)) {
+			instr->state = MTD_ERASE_FAILED;
+			mutex_unlock(&flash->lock);
+			return -EIO;
+		}
 
 	/* REVISIT in some cases we could speed up erasing large regions
 	 * by using OPCODE_SE instead of OPCODE_BE_4K.  We may have set up

Re: [PATCH] Bug in m25p80.c during whole-chip erase

[Mike Frysinger]

On Mon, Apr 27, 2009 at 17:10, Steven A. Falco wrote:
> There is a logic error in "whole chip erase" for the m25p80 family.  If
> the whole device is successfully erased, erase_chip() will return 0, and
> the code will fall through to the "else" clause, and do sector-by-sector
> erase in addition to the whole-chip erase.  This patch corrects that.
>
> Also, the MAX_READY_WAIT_COUNT is insufficient for an m25p16 connected
> to a 400 MHz powerpc.  Increasing it allows me to successfully program
> the device on my board.

in general, trying to set timeouts "close" to the spec gains nothing
with spi flash devices.  the timeout limit is hit only when an error
occurs, and errors should not occur during the normal run of things.
on the other side, having a timeout be wrongly hit when the device is
operating correctly is a much worse situation to be in.
-mike

Re: [PATCH] Bug in m25p80.c during whole-chip erase

[Steven A. Falco]

Mike Frysinger wrote:
> On Mon, Apr 27, 2009 at 17:10, Steven A. Falco wrote:
>> There is a logic error in "whole chip erase" for the m25p80 family.  If
>> the whole device is successfully erased, erase_chip() will return 0, and
>> the code will fall through to the "else" clause, and do sector-by-sector
>> erase in addition to the whole-chip erase.  This patch corrects that.
>>
>> Also, the MAX_READY_WAIT_COUNT is insufficient for an m25p16 connected
>> to a 400 MHz powerpc.  Increasing it allows me to successfully program
>> the device on my board.
> 
> in general, trying to set timeouts "close" to the spec gains nothing
> with spi flash devices.  the timeout limit is hit only when an error
> occurs, and errors should not occur during the normal run of things.
> on the other side, having a timeout be wrongly hit when the device is
> operating correctly is a much worse situation to be in.
> -mike
> 

Right.  This chip takes 13 seconds (typical) to bulk erase, according to
the Numonyx data sheet.  So increasing the timeout to account for such
a slow part is necessary, and allows me to successfully erase the part.

Are there any changes you'd like to see in this patch, or is it ok as
written?

	Steve

Re: [PATCH] Bug in m25p80.c during whole-chip erase

[Steven A. Falco]

Peter Horton wrote:
> 
> Steven A. Falco wrote:
>> Mike Frysinger wrote:
>>> On Mon, Apr 27, 2009 at 17:10, Steven A. Falco wrote:
>>>> There is a logic error in "whole chip erase" for the m25p80 family.  If
>>>> the whole device is successfully erased, erase_chip() will return 0,
>>>> and
>>>> the code will fall through to the "else" clause, and do
>>>> sector-by-sector
>>>> erase in addition to the whole-chip erase.  This patch corrects that.
>>>>
>>>> Also, the MAX_READY_WAIT_COUNT is insufficient for an m25p16 connected
>>>> to a 400 MHz powerpc.  Increasing it allows me to successfully program
>>>> the device on my board.
>>> in general, trying to set timeouts "close" to the spec gains nothing
>>> with spi flash devices.  the timeout limit is hit only when an error
>>> occurs, and errors should not occur during the normal run of things.
>>> on the other side, having a timeout be wrongly hit when the device is
>>> operating correctly is a much worse situation to be in.
>>> -mike
>>>
>>
>> Right.  This chip takes 13 seconds (typical) to bulk erase, according to
>> the Numonyx data sheet.  So increasing the timeout to account for such
>> a slow part is necessary, and allows me to successfully erase the part.
>>
>> Are there any changes you'd like to see in this patch, or is it ok as
>> written?
>>
> 
> Check the list archives. I submitted a patch for this a few weeks ago
> which has been merged into AKPMs tree.
> 
> P.
> 

Your patch addresses the timing issues, but not the duplicate erase.

I'll resubmit my patch with just the duplicate erase fix, as this is
still needed.

	Steve

Re: [PATCH] Bug in m25p80.c during whole-chip erase

[Mike Frysinger]

On Tue, Apr 28, 2009 at 09:40, Steven A. Falco wrote:
> Mike Frysinger wrote:
>> On Mon, Apr 27, 2009 at 17:10, Steven A. Falco wrote:
>>> There is a logic error in "whole chip erase" for the m25p80 family.  If
>>> the whole device is successfully erased, erase_chip() will return 0, and
>>> the code will fall through to the "else" clause, and do sector-by-sector
>>> erase in addition to the whole-chip erase.  This patch corrects that.
>>>
>>> Also, the MAX_READY_WAIT_COUNT is insufficient for an m25p16 connected
>>> to a 400 MHz powerpc.  Increasing it allows me to successfully program
>>> the device on my board.
>>
>> in general, trying to set timeouts "close" to the spec gains nothing
>> with spi flash devices.  the timeout limit is hit only when an error
>> occurs, and errors should not occur during the normal run of things.
>> on the other side, having a timeout be wrongly hit when the device is
>> operating correctly is a much worse situation to be in.
>
> Right.  This chip takes 13 seconds (typical) to bulk erase, according to
> the Numonyx data sheet.  So increasing the timeout to account for such
> a slow part is necessary, and allows me to successfully erase the part.
>
> Are there any changes you'd like to see in this patch, or is it ok as
> written?

changes look fine to me, but i'm not in any position to get things
merged for you ;)
-mike