| Message ID | 49F61F32.3020003@harris.com |
|---|---|
| State | Accepted |
| Commit | 3f33b0aaac4e208579fe5aa2964857d4e9ba10c5 |
| Headers | show |
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
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
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
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
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
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> ---