From 6189d6bdc5b49db87ad87a502807e56d0288d652 Mon Sep 17 00:00:00 2001 From: Dominic Clifton Date: Sun, 9 Jun 2019 17:35:21 +0200 Subject: [PATCH 1/2] Fix Flash API timeout issues. Flash operations can specify how long to wait before the next operation is issued. Prior to this the amount of time waited, and when, was wrong. e.g. m25p16_eraseCompletely - possibly waits ages TO START, starts, exit. m25p16_pageProgramContinue - waits DEFAULT_TIMEOUT_MILLIS to START, starts, exits. m25p16_pageProgramContinue would fail to write to the flash as the device was still busy erasing and didn't wait long enough. what happens now is: m25p16_eraseCompletely - waits using the current timeout, starts, sets timeout to be `now + BULK_ERASE_TIMEOUT_MILLIS`, exits. m25p16_pageProgramContinue - waits using the current `BULK_ERASE_TIMEOUT_MILLIS`, starts, exists, sets timeout to be `now + DEFAULT_TIMEOUT_MILLIS`. Since the timeout is stored in the flashDevice_t the solution also works for multi-die devices which use an instance of flashDevice_t for each die. --- src/main/drivers/flash.c | 8 +++-- src/main/drivers/flash.h | 2 +- src/main/drivers/flash_impl.h | 3 +- src/main/drivers/flash_m25p16.c | 31 ++++++++++++----- src/main/drivers/flash_w25m.c | 10 +++--- src/main/drivers/flash_w25n01g.c | 59 ++++++++++++++++++++++---------- 6 files changed, 74 insertions(+), 39 deletions(-) diff --git a/src/main/drivers/flash.c b/src/main/drivers/flash.c index 97aa946f1a..863f51f5cc 100644 --- a/src/main/drivers/flash.c +++ b/src/main/drivers/flash.c @@ -205,9 +205,9 @@ bool flashIsReady(void) return flashDevice.vTable->isReady(&flashDevice); } -bool flashWaitForReady(uint32_t timeoutMillis) +bool flashWaitForReady(void) { - return flashDevice.vTable->waitForReady(&flashDevice, timeoutMillis); + return flashDevice.vTable->waitForReady(&flashDevice); } void flashEraseSector(uint32_t address) @@ -247,7 +247,9 @@ int flashReadBytes(uint32_t address, uint8_t *buffer, int length) void flashFlush(void) { - flashDevice.vTable->flush(&flashDevice); + if (flashDevice.vTable->flush) { + flashDevice.vTable->flush(&flashDevice); + } } static const flashGeometry_t noFlashGeometry = { diff --git a/src/main/drivers/flash.h b/src/main/drivers/flash.h index dac4df70bf..dc232762c6 100644 --- a/src/main/drivers/flash.h +++ b/src/main/drivers/flash.h @@ -51,7 +51,7 @@ void flashPreInit(const flashConfig_t *flashConfig); bool flashInit(const flashConfig_t *flashConfig); bool flashIsReady(void); -bool flashWaitForReady(uint32_t timeoutMillis); +bool flashWaitForReady(void); void flashEraseSector(uint32_t address); void flashEraseCompletely(void); void flashPageProgramBegin(uint32_t address); diff --git a/src/main/drivers/flash_impl.h b/src/main/drivers/flash_impl.h index af1f7197a7..691765f697 100644 --- a/src/main/drivers/flash_impl.h +++ b/src/main/drivers/flash_impl.h @@ -53,12 +53,13 @@ typedef struct flashDevice_s { // for writes. This allows us to avoid polling for writable status // when it is definitely ready already. bool couldBeBusy; + uint32_t timeoutAt; flashDeviceIO_t io; } flashDevice_t; typedef struct flashVTable_s { bool (*isReady)(flashDevice_t *fdevice); - bool (*waitForReady)(flashDevice_t *fdevice, uint32_t timeoutMillis); + bool (*waitForReady)(flashDevice_t *fdevice); void (*eraseSector)(flashDevice_t *fdevice, uint32_t address); void (*eraseCompletely)(flashDevice_t *fdevice); void (*pageProgramBegin)(flashDevice_t *fdevice, uint32_t address); diff --git a/src/main/drivers/flash_m25p16.c b/src/main/drivers/flash_m25p16.c index a194c08181..88d879af6f 100644 --- a/src/main/drivers/flash_m25p16.c +++ b/src/main/drivers/flash_m25p16.c @@ -150,15 +150,22 @@ static bool m25p16_isReady(flashDevice_t *fdevice) return !fdevice->couldBeBusy; } -static bool m25p16_waitForReady(flashDevice_t *fdevice, uint32_t timeoutMillis) +static void m25p16_setTimeout(flashDevice_t *fdevice, uint32_t timeoutMillis) +{ + uint32_t now = millis(); + fdevice->timeoutAt = now + timeoutMillis; +} + +static bool m25p16_waitForReady(flashDevice_t *fdevice) { - uint32_t time = millis(); while (!m25p16_isReady(fdevice)) { - if (millis() - time > timeoutMillis) { + uint32_t now = millis(); + if (cmp32(now, fdevice->timeoutAt) >= 0) { return false; } } + fdevice->timeoutAt = 0; return true; } @@ -246,20 +253,24 @@ static void m25p16_eraseSector(flashDevice_t *fdevice, uint32_t address) m25p16_setCommandAddress(&out[1], address, fdevice->isLargeFlash); - m25p16_waitForReady(fdevice, SECTOR_ERASE_TIMEOUT_MILLIS); + m25p16_waitForReady(fdevice); m25p16_writeEnable(fdevice); m25p16_transfer(fdevice->io.handle.busdev, out, NULL, fdevice->isLargeFlash ? 5 : 4); + + m25p16_setTimeout(fdevice, SECTOR_ERASE_TIMEOUT_MILLIS); } static void m25p16_eraseCompletely(flashDevice_t *fdevice) { - m25p16_waitForReady(fdevice, BULK_ERASE_TIMEOUT_MILLIS); + m25p16_waitForReady(fdevice); m25p16_writeEnable(fdevice); m25p16_performOneByteCommand(fdevice->io.handle.busdev, M25P16_INSTRUCTION_BULK_ERASE); + + m25p16_setTimeout(fdevice, BULK_ERASE_TIMEOUT_MILLIS); } static void m25p16_pageProgramBegin(flashDevice_t *fdevice, uint32_t address) @@ -275,7 +286,7 @@ static void m25p16_pageProgramContinue(flashDevice_t *fdevice, const uint8_t *da m25p16_setCommandAddress(&command[1], fdevice->currentWriteAddress, fdevice->isLargeFlash); - m25p16_waitForReady(fdevice, DEFAULT_TIMEOUT_MILLIS); + m25p16_waitForReady(fdevice); m25p16_writeEnable(fdevice); @@ -295,6 +306,8 @@ static void m25p16_pageProgramContinue(flashDevice_t *fdevice, const uint8_t *da #endif fdevice->currentWriteAddress += length; + + m25p16_setTimeout(fdevice, DEFAULT_TIMEOUT_MILLIS); } static void m25p16_pageProgramFinish(flashDevice_t *fdevice) @@ -330,8 +343,6 @@ static void m25p16_pageProgram(flashDevice_t *fdevice, uint32_t address, const u * Read `length` bytes into the provided `buffer` from the flash starting from the given `address` (which need not lie * on a page boundary). * - * Waits up to DEFAULT_TIMEOUT_MILLIS milliseconds for the flash to become ready before reading. - * * The number of bytes actually read is returned, which can be zero if an error or timeout occurred. */ static int m25p16_readBytes(flashDevice_t *fdevice, uint32_t address, uint8_t *buffer, int length) @@ -340,7 +351,7 @@ static int m25p16_readBytes(flashDevice_t *fdevice, uint32_t address, uint8_t *b m25p16_setCommandAddress(&command[1], address, fdevice->isLargeFlash); - if (!m25p16_waitForReady(fdevice, DEFAULT_TIMEOUT_MILLIS)) { + if (!m25p16_waitForReady(fdevice)) { return 0; } @@ -359,6 +370,8 @@ static int m25p16_readBytes(flashDevice_t *fdevice, uint32_t address, uint8_t *b m25p16_disable(fdevice->io.handle.busdev); #endif + m25p16_setTimeout(fdevice, DEFAULT_TIMEOUT_MILLIS); + return length; } diff --git a/src/main/drivers/flash_w25m.c b/src/main/drivers/flash_w25m.c index aefa7b4b40..d8cf88e12d 100644 --- a/src/main/drivers/flash_w25m.c +++ b/src/main/drivers/flash_w25m.c @@ -80,8 +80,6 @@ static void w25m_dieSelect(busDevice_t *busdev, int die) static bool w25m_isReady(flashDevice_t *fdevice) { - UNUSED(fdevice); - for (int die = 0 ; die < dieCount ; die++) { if (dieDevice[die].couldBeBusy) { w25m_dieSelect(fdevice->io.handle.busdev, die); @@ -94,11 +92,11 @@ static bool w25m_isReady(flashDevice_t *fdevice) return true; } -static bool w25m_waitForReady(flashDevice_t *fdevice, uint32_t timeoutMillis) +static bool w25m_waitForReady(flashDevice_t *fdevice) { - uint32_t time = millis(); - while (!w25m_isReady(fdevice)) { - if (millis() - time > timeoutMillis) { + for (int die = 0 ; die < dieCount ; die++) { + w25m_dieSelect(fdevice->io.handle.busdev, die); + if (!dieDevice[die].vTable->waitForReady(&dieDevice[die])) { return false; } } diff --git a/src/main/drivers/flash_w25n01g.c b/src/main/drivers/flash_w25n01g.c index 0cb3f287af..585402b496 100644 --- a/src/main/drivers/flash_w25n01g.c +++ b/src/main/drivers/flash_w25n01g.c @@ -143,8 +143,13 @@ typedef struct bblut_s { #define DISABLE(busdev) IOHi((busdev)->busdev_u.spi.csnPin); __NOP() #define ENABLE(busdev) __NOP(); IOLo((busdev)->busdev_u.spi.csnPin) -// XXX remove - add a forward declaration to keep git diff small while this is work-in-progress. -bool w25n01g_waitForReady(flashDevice_t *fdevice, uint32_t timeoutMillis); +static bool w25n01g_waitForReady(flashDevice_t *fdevice); + +static void w25n01g_setTimeout(flashDevice_t *fdevice, uint32_t timeoutMillis) +{ + uint32_t now = millis(); + fdevice->timeoutAt = now + timeoutMillis; +} /** * Send the given command byte to the device. @@ -241,7 +246,8 @@ static void w25n01g_deviceReset(flashDevice_t *fdevice) w25n01g_performOneByteCommand(io, W25N01G_INSTRUCTION_DEVICE_RESET); - w25n01g_waitForReady(fdevice, W25N01G_TIMEOUT_RESET_MS); + w25n01g_setTimeout(fdevice, W25N01G_TIMEOUT_RESET_MS); + w25n01g_waitForReady(fdevice); // Protection for upper 1/32 (BP[3:0] = 0101, TB=0), WP-E on; to protect bad block replacement area // DON'T DO THIS. This will prevent writes through the bblut as well. @@ -293,15 +299,16 @@ bool w25n01g_isReady(flashDevice_t *fdevice) #endif } -bool w25n01g_waitForReady(flashDevice_t *fdevice, uint32_t timeoutMillis) +static bool w25n01g_waitForReady(flashDevice_t *fdevice) { - uint32_t time = millis(); while (!w25n01g_isReady(fdevice)) { - if (millis() - time > timeoutMillis) { + uint32_t now = millis(); + if (cmp32(now, fdevice->timeoutAt) >= 0) { DPRINTF(("*** TIMEOUT %d\r\n", timeoutMillis)); return false; } } + fdevice->timeoutAt = 0; return true; } @@ -420,11 +427,13 @@ bool w25n01g_detect(flashDevice_t *fdevice, uint32_t chipID) void w25n01g_eraseSector(flashDevice_t *fdevice, uint32_t address) { - w25n01g_waitForReady(fdevice, W25N01G_TIMEOUT_BLOCK_ERASE_MS); + w25n01g_waitForReady(fdevice); w25n01g_writeEnable(fdevice); w25n01g_performCommandWithPageAddress(&fdevice->io, W25N01G_INSTRUCTION_BLOCK_ERASE, W25N01G_LINEAR_TO_PAGE(address)); + + w25n01g_setTimeout(fdevice, W25N01G_TIMEOUT_BLOCK_ERASE_MS); } // @@ -442,7 +451,7 @@ static void w25n01g_programDataLoad(flashDevice_t *fdevice, uint16_t columnAddre { //DPRINTF((" load WaitForReady\r\n")); - w25n01g_waitForReady(fdevice, W25N01G_TIMEOUT_PAGE_PROGRAM_MS); + w25n01g_waitForReady(fdevice); //DPRINTF((" load Issuing command\r\n")); @@ -463,6 +472,8 @@ static void w25n01g_programDataLoad(flashDevice_t *fdevice, uint16_t columnAddre } #endif //DPRINTF((" load Done\r\n")); + + w25n01g_setTimeout(fdevice, W25N01G_TIMEOUT_PAGE_PROGRAM_MS); } static void w25n01g_randomProgramDataLoad(flashDevice_t *fdevice, uint16_t columnAddress, const uint8_t *data, int length) @@ -470,7 +481,7 @@ static void w25n01g_randomProgramDataLoad(flashDevice_t *fdevice, uint16_t colum const uint8_t cmd[] = { W25N01G_INSTRUCTION_RANDOM_PROGRAM_DATA_LOAD, columnAddress >> 8, columnAddress & 0xff }; //DPRINTF((" random WaitForReady\r\n")); - w25n01g_waitForReady(fdevice, W25N01G_TIMEOUT_PAGE_PROGRAM_MS); + w25n01g_waitForReady(fdevice); //DPRINTF((" random Issuing command\r\n")); if (fdevice->io.mode == FLASHIO_SPI) { @@ -491,18 +502,22 @@ static void w25n01g_randomProgramDataLoad(flashDevice_t *fdevice, uint16_t colum #endif //DPRINTF((" random Done\r\n")); + + w25n01g_setTimeout(fdevice, W25N01G_TIMEOUT_PAGE_PROGRAM_MS); + } static void w25n01g_programExecute(flashDevice_t *fdevice, uint32_t pageAddress) { //DPRINTF((" execute WaitForReady\r\n")); - w25n01g_waitForReady(fdevice, W25N01G_TIMEOUT_PAGE_PROGRAM_MS); + w25n01g_waitForReady(fdevice); //DPRINTF((" execute Issuing command\r\n")); w25n01g_performCommandWithPageAddress(&fdevice->io, W25N01G_INSTRUCTION_PROGRAM_EXECUTE, pageAddress); //DPRINTF((" execute Done\r\n")); + w25n01g_setTimeout(fdevice, W25N01G_TIMEOUT_PAGE_PROGRAM_MS); } // @@ -557,7 +572,7 @@ void w25n01g_pageProgramBegin(flashDevice_t *fdevice, uint32_t address) if (address != programLoadAddress) { PAGEPROG_DPRINTF((" Buffer dirty and address != programLoadAddress (0x%x), flushing\r\n", programLoadAddress)); PAGEPROG_DPRINTF((" Wait for ready\r\n")); - w25n01g_waitForReady(fdevice, W25N01G_TIMEOUT_PAGE_PROGRAM_MS); + w25n01g_waitForReady(fdevice); isProgramming = false; @@ -589,7 +604,7 @@ void w25n01g_pageProgramContinue(flashDevice_t *fdevice, const uint8_t *data, in } PAGEPROG_DPRINTF((" Wait for ready\r\n")); - w25n01g_waitForReady(fdevice, W25N01G_TIMEOUT_PAGE_PROGRAM_MS); + w25n01g_waitForReady(fdevice); PAGEPROG_DPRINTF((" Write enable\r\n")); w25n01g_writeEnable(fdevice); @@ -715,7 +730,7 @@ int w25n01g_readBytes(flashDevice_t *fdevice, uint32_t address, uint8_t *buffer, READBYTES_DPRINTF(("readBytes: PAGE_DATA_READ page 0x%x\r\n", targetPage)); - if (!w25n01g_waitForReady(fdevice, W25N01G_TIMEOUT_PAGE_READ_MS)) { + if (!w25n01g_waitForReady(fdevice)) { return 0; } @@ -723,7 +738,8 @@ int w25n01g_readBytes(flashDevice_t *fdevice, uint32_t address, uint8_t *buffer, w25n01g_performCommandWithPageAddress(&fdevice->io, W25N01G_INSTRUCTION_PAGE_DATA_READ, targetPage); - if (!w25n01g_waitForReady(fdevice, W25N01G_TIMEOUT_PAGE_READ_MS)) { + w25n01g_setTimeout(fdevice, W25N01G_TIMEOUT_PAGE_READ_MS); + if (!w25n01g_waitForReady(fdevice)) { return 0; } @@ -766,7 +782,8 @@ int w25n01g_readBytes(flashDevice_t *fdevice, uint32_t address, uint8_t *buffer, #endif // XXX Don't need this? - if (!w25n01g_waitForReady(fdevice, W25N01G_TIMEOUT_PAGE_READ_MS)) { + w25n01g_setTimeout(fdevice, W25N01G_TIMEOUT_PAGE_READ_MS); + if (!w25n01g_waitForReady(fdevice)) { return 0; } @@ -794,12 +811,12 @@ int w25n01g_readBytes(flashDevice_t *fdevice, uint32_t address, uint8_t *buffer, int w25n01g_readExtensionBytes(flashDevice_t *fdevice, uint32_t address, uint8_t *buffer, int length) { - w25n01g_performCommandWithPageAddress(&fdevice->io, W25N01G_INSTRUCTION_PAGE_DATA_READ, W25N01G_LINEAR_TO_PAGE(address)); - - if (!w25n01g_waitForReady(fdevice, W25N01G_TIMEOUT_PAGE_READ_MS)) { + if (!w25n01g_waitForReady(fdevice)) { return 0; } + w25n01g_performCommandWithPageAddress(&fdevice->io, W25N01G_INSTRUCTION_PAGE_DATA_READ, W25N01G_LINEAR_TO_PAGE(address)); + uint32_t column = 2048; if (fdevice->io.mode == FLASHIO_SPI) { @@ -825,6 +842,8 @@ int w25n01g_readExtensionBytes(flashDevice_t *fdevice, uint32_t address, uint8_t } #endif + w25n01g_setTimeout(fdevice, W25N01G_TIMEOUT_PAGE_READ_MS); + return length; } @@ -899,6 +918,8 @@ void w25n01g_readBBLUT(flashDevice_t *fdevice, bblut_t *bblut, int lutsize) void w25n01g_writeBBLUT(flashDevice_t *fdevice, uint16_t lba, uint16_t pba) { + w25n01g_waitForReady(fdevice); + if (fdevice->io.mode == FLASHIO_SPI) { busDevice_t *busdev = fdevice->io.handle.busdev; @@ -917,7 +938,7 @@ void w25n01g_writeBBLUT(flashDevice_t *fdevice, uint16_t lba, uint16_t pba) } #endif - w25n01g_waitForReady(fdevice, W25N01G_TIMEOUT_PAGE_PROGRAM_MS); + w25n01g_setTimeout(fdevice, W25N01G_TIMEOUT_PAGE_PROGRAM_MS); } static void w25n01g_deviceInit(flashDevice_t *flashdev) From b09012621bafa58a66557a21a05a0bdf9d2c99bf Mon Sep 17 00:00:00 2001 From: Dominic Clifton Date: Wed, 12 Jun 2019 12:22:07 +0200 Subject: [PATCH 2/2] Update default flash bulk-erase timeout and improve developer documentation regarding timeouts. --- src/main/drivers/flash_m25p16.c | 8 +++++--- src/main/drivers/flash_w25n01g.c | 2 ++ 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/main/drivers/flash_m25p16.c b/src/main/drivers/flash_m25p16.c index 88d879af6f..8462a999d9 100644 --- a/src/main/drivers/flash_m25p16.c +++ b/src/main/drivers/flash_m25p16.c @@ -68,12 +68,14 @@ #define JEDEC_ID_CYPRESS_S25FL128L 0x016018 #define JEDEC_ID_BERGMICRO_W25Q32 0xE04016 +// IMPORTANT: Timeout values are currently required to be set to the highest value required by any of the supported flash chips by this driver. + // The timeout we expect between being able to issue page program instructions #define DEFAULT_TIMEOUT_MILLIS 6 - -// These take sooooo long: #define SECTOR_ERASE_TIMEOUT_MILLIS 5000 -#define BULK_ERASE_TIMEOUT_MILLIS 21000 + +// etracer65 notes: For bulk erase The 25Q16 takes about 3 seconds and the 25Q128 takes about 49 +#define BULK_ERASE_TIMEOUT_MILLIS 50000 #define M25P16_PAGESIZE 256 diff --git a/src/main/drivers/flash_w25n01g.c b/src/main/drivers/flash_w25n01g.c index 585402b496..01fd26b807 100644 --- a/src/main/drivers/flash_w25n01g.c +++ b/src/main/drivers/flash_w25n01g.c @@ -122,6 +122,8 @@ serialPort_t *debugSerialPort = NULL; #define W25N01G_BLOCK_TO_PAGE(block) ((block) * W25N01G_PAGES_PER_BLOCK) #define W25N01G_BLOCK_TO_LINEAR(block) (W25N01G_BLOCK_TO_PAGE(block) * W25N01G_PAGE_SIZE) +// IMPORTANT: Timeout values are currently required to be set to the highest value required by any of the supported flash chips by this driver + // The timeout values (2ms minimum to avoid 1 tick advance in consecutive calls to millis). #define W25N01G_TIMEOUT_PAGE_READ_MS 2 // tREmax = 60us (ECC enabled) #define W25N01G_TIMEOUT_PAGE_PROGRAM_MS 2 // tPPmax = 700us