mtd_spi_nor: Use SFDP for SPI NOR flash parameter autodiscovery #15617
+410
−55
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bergzand
added
Type: new feature
bergzand
requested review from
aabadie,
basilfx and
haukepetersen
as code owners
bergzand
force-pushed
the
pr/mtd_spi_nor/sfdp
branch
3 times, most recently
from
b83ec2f to
290843d
Compare
bergzand
force-pushed
the
pr/mtd_spi_nor/sfdp
branch
from
a9e0601 to
a7d2273
Compare
bergzand
added
CI: ready for build
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions. |
stale
bot
added
the
State: stale
bergzand
added
State: don't stale
|
I would still like to have this! |
JEDEC has a standardized device information tables for serial flash chips. This commit adds limited support for reading these tables and configure the flash parameters based on these tables. A flag is added to disable this to allow for manual configuration. A downside of this change is that most SPI NOR flash parameters have to reside in the MCU memory.
bergzand
force-pushed
the
pr/mtd_spi_nor/sfdp
branch
from
a7d2273 to
3bed1e0
Compare
|
rebased! |
crasbe
reviewed
Nov 21, 2024
| mtd_jedec_id_t jedec_id; /**< JEDEC ID of the chip */ | ||
|
|
||
| uint8_t addr_width; /**< Number of bytes in addresses, usually 3 for small devices */ |
There was a problem hiding this comment.
This is a duplicate of the addr_width member in line 168.
Comment on lines
+443
to
+451
| /* Read the useful parameters from the basic SPI flash param table */ | ||
|
|
||
| _read_sfdp_access(dev, param); | ||
|
|
||
| /* Size of the flash */ | ||
| _read_sfdp_size(dev, param); | ||
|
|
||
| /* Erase timings */ | ||
|
|
There was a problem hiding this comment.
I think the /* Erase timings */ comment should be above the _read_sfdp_access() function, at least that is where the erase timings are written to the device structure.
| mtd_dev_t *mtd = &dev->base; | ||
| mtd->sector_count = size / (mtd->pages_per_sector * mtd->page_size); | ||
|
|
||
| DEBUG("Full size: %"PRIu32", sector_count = %"PRIu32"\n", size, mtd->sector_count); |
There was a problem hiding this comment.
It would be good to add mtd_spi_sfdp: in front of this debug message as well for consistency.
Comment on lines
+383
to
+384
| DEBUG("Erase type %u: %"PRIu32"B, inst: 0x%.2x, erase time: %"PRIu32"ms\n", | ||
| i + 1, 1LU << (access_info.erase[i].size), access_info.erase[i].instruction, _sfdp_sector_erase_timing(access_info.erase_time >> (4 + 7 * i))); |
There was a problem hiding this comment.
It would be good to add mtd_spi_sfdp: in front of this debug message as well for consistency.
| size_t size_exp = access_info.erase[i].size; | ||
|
|
||
| if (!size_exp) { | ||
| DEBUG("Empty erase time record, skipping\n"); |
There was a problem hiding this comment.
It would be good to add mtd_spi_sfdp: in front of this debug message as well for consistency.
|
I gave this a test with my nRF52840DK and the SFDP functionality seems to work with the on board Macronix MX25R6432F chip: It correctly detected that it is 8MB big and has 4096 byte sectors. Log with mtd_spi_nor debug messages enabled:However the Log with mtd_spi_nor debug messages enabled:Then I tested it with the IS25LE01G, which was also successful. Likewise, it shows the occupation in sectors. Log with mtd_spi_nor debug messages enabled:The IS25LP128 does not seem to work. The size was detected correctly, but the formatting failed. I Log with mtd_spi_nor debug messages enabled:The formatting does not seem to work with the MX25L12873F either: Log with mtd_spi_nor debug messages enabled:HOWEVER, the same errors happen with the master branch version that your addition is based on. I reset it with In the current master, both flash chips work as they should, so I guess something changed in the meantime and a rebase would fix it. I did not check yet how many things changed in the meantime and how hard a rebase would be. I agree with @benpicco that this is an awesome addition to the mtd_spi_nor driver. |
crasbe
reviewed
Jan 16, 2025
| .wait_32k_erase = 20LU * US_PER_MS, | ||
| .wait_chip_wake_up = 1LU * US_PER_MS, | ||
| .flag = SPI_NOR_F_NO_SFDP, | ||
| .addr_width = 3, |
There was a problem hiding this comment.
Suggested change
| .addr_width = 3, |
This should not be necessary and will be overwritten by _set_addr_width in mtd_spi_nor_init anyway.
|
This PR can actually still be rebased to the current master with limited changes and it solves the aforementioned issues. The patch was created with the following command: Patch for mtd_spi_nor.cdiff --git a/drivers/mtd_spi_nor/mtd_spi_nor.c b/drivers/mtd_spi_nor/mtd_spi_nor.c
index 7ba3007132..96d217a8b8 100644
--- a/drivers/mtd_spi_nor/mtd_spi_nor.c
+++ b/drivers/mtd_spi_nor/mtd_spi_nor.c
@@ -25,15 +25,22 @@
#include <string.h>
#include <errno.h>
-#include "macros/units.h"
-#include "mtd.h"
-#include "xtimer.h"
-#include "timex.h"
-#include "thread.h"
+#include "busy_wait.h"
#include "byteorder.h"
-#include "bitarithm.h"
+#include "kernel_defines.h"
+#include "macros/math.h"
+#include "macros/utils.h"
+#include "mtd.h"
#include "mtd_spi_nor.h"
#include "mtd_spi_nor/sfdp.h"
+#include "time_units.h"
+#include "thread.h"
+
+#if IS_USED(MODULE_ZTIMER)
+#include "ztimer.h"
+#elif IS_USED(MODULE_XTIMER)
+#include "xtimer.h"
+#endif
#define ENABLE_DEBUG 0
#include "debug.h"
@@ -60,8 +67,6 @@
#define MBIT_AS_BYTES ((1024 * 1024) / 8)
-#define MIN(a, b) ((a) > (b) ? (b) : (a))
-
/**
* @brief JEDEC memory manufacturer ID codes.
*
@@ -392,15 +397,15 @@ static void _read_sfdp_access(mtd_spi_nor_t *dev,
size_t size = 1LU << size_exp;
uint32_t timing = _sfdp_sector_erase_timing(access_info.erase_time >> (4 + 7 * i));
switch (size) {
- case KiB(4):
+ case 4096:
dev->wait_sector_erase = timing;
dev->flag |= SPI_NOR_F_SECT_4K;
break;
- case KiB(32):
+ case 32768:
dev->wait_32k_erase = timing;
dev->flag |= SPI_NOR_F_SECT_32K;
break;
- case KiB(64):
+ case 65536:
dev->wait_64k_erase = timing;
dev->flag |= SPI_NOR_F_SECT_64K;
break;
@@ -514,7 +519,8 @@ static uint32_t mtd_spi_nor_get_size(const mtd_jedec_id_t *id)
if (mtd_spi_manuf_match(id, SPI_NOR_JEDEC_ATMEL) &&
/* ID 2 is used to encode the product version, usually 1 or 2 */
(id->device[1] & ~0x3) == 0) {
- return (0x1F & id->device[0]) * MBIT_AS_BYTES;
+ /* capacity encoded as power of 32k sectors */
+ return (32 * 1024) << (0x1F & id->device[0]);
}
if (mtd_spi_manuf_match(id, SPI_NOR_JEDEC_MICROCHIP)) {
switch (id->device[1]) {
@@ -543,14 +549,29 @@ static uint32_t mtd_spi_nor_get_size(const mtd_jedec_id_t *id)
return 1 << id->device[1];
}
+static void delay_us(unsigned us)
+{
+#if defined(MODULE_ZTIMER_USEC)
+ ztimer_sleep(ZTIMER_USEC, us);
+#elif defined(MODULE_ZTIMER_MSEC)
+ ztimer_sleep(ZTIMER_MSEC, DIV_ROUND_UP(us, US_PER_MS));
+#else
+ busy_wait_us(us);
+#endif
+}
+
static inline void wait_for_write_complete(const mtd_spi_nor_t *dev, uint32_t us)
{
unsigned i = 0, j = 0;
- uint32_t div = 2;
+ uint32_t div = 1; /* first wait one full interval */
+#if IS_ACTIVE(ENABLE_DEBUG)
uint32_t diff = 0;
- if (IS_ACTIVE(ENABLE_DEBUG) && IS_USED(MODULE_XTIMER)) {
- diff = xtimer_now_usec();
- }
+#endif
+#if IS_ACTIVE(ENABLE_DEBUG) && IS_USED(MODULE_ZTIMER_USEC)
+ diff = ztimer_now(ZTIMER_USEC);
+#elif IS_ACTIVE(ENABLE_DEBUG) && IS_USED(MODULE_XTIMER)
+ diff = xtimer_now_usec();
+#endif
do {
uint8_t status;
mtd_spi_cmd_read(dev, dev->params->opcode->rdsr, &status, sizeof(status));
@@ -560,34 +581,31 @@ static inline void wait_for_write_complete(const mtd_spi_nor_t *dev, uint32_t us
break;
}
i++;
-#if MODULE_XTIMER
if (us) {
- xtimer_usleep(us);
+ uint32_t wait_us = us / div;
+ uint32_t wait_min = 2;
+
+ wait_us = wait_us > wait_min ? wait_us : wait_min;
+
+ delay_us(wait_us);
/* reduce the waiting time quickly if the estimate was too short,
* but still avoid busy (yield) waiting */
- if (us > 2 * XTIMER_BACKOFF) {
- us -= (us / div);
- div++;
- }
- else {
- us = 2 * XTIMER_BACKOFF;
- }
+ div++;
}
else {
j++;
thread_yield();
}
-#else
- (void)div;
- (void) us;
- thread_yield();
-#endif
} while (1);
DEBUG("wait loop %u times, yield %u times", i, j);
- if (IS_ACTIVE(ENABLE_DEBUG) && IS_ACTIVE(MODULE_XTIMER)) {
- diff = xtimer_now_usec() - diff;
- DEBUG(", total wait %"PRIu32"us", diff);
- }
+#if IS_ACTIVE(ENABLE_DEBUG)
+#if IS_USED(MODULE_ZTIMER_USEC)
+ diff = ztimer_now(ZTIMER_USEC) - diff;
+#elif IS_USED(MODULE_XTIMER)
+ diff = xtimer_now_usec() - diff;
+#endif
+ DEBUG(", total wait %"PRIu32"us", diff);
+#endif
DEBUG("\n");
}
@@ -625,20 +643,22 @@ static int mtd_spi_nor_power(mtd_dev_t *mtd, enum mtd_power_state power)
switch (power) {
case MTD_POWER_UP:
mtd_spi_cmd(dev, dev->params->opcode->wake);
-#if defined(MODULE_XTIMER)
- /* No sense in trying multiple times if no xtimer to wait between
- reads */
- uint8_t retries = 0;
+
+ /* fall back to polling if no timer is used */
+ unsigned retries = MTD_POWER_UP_WAIT_FOR_ID;
+ if (!IS_USED(MODULE_ZTIMER) && !IS_USED(MODULE_XTIMER)) {
+ retries *= dev->wait_chip_wake_up * 1000;
+ }
+
int res = 0;
do {
- xtimer_usleep(dev->wait_chip_wake_up);
+ delay_us(dev->wait_chip_wake_up);
res = mtd_spi_read_jedec_id(dev, &dev->jedec_id);
- retries++;
- } while (res < 0 && retries < MTD_POWER_UP_WAIT_FOR_ID);
+ } while (res < 0 && --retries);
if (res < 0) {
+ mtd_spi_release(dev);
return -EIO;
}
-#endif
/* enable 32 bit address mode */
if (dev->addr_width == 4) {
_enable_32bit_addr(dev);
@@ -661,7 +681,7 @@ static void _set_addr_width(mtd_dev_t *mtd)
uint32_t flash_size = mtd->pages_per_sector * mtd->page_size
* mtd->sector_count;
- if (flash_size > 0xFFFFFF) {
+ if (flash_size > (0x1UL << 24)) {
dev->addr_width = 4;
} else {
dev->addr_width = 3;
@@ -680,9 +700,9 @@ static int mtd_spi_nor_init(mtd_dev_t *mtd)
_init_pins(dev);
/* power up the MTD device*/
- DEBUG("mtd_spi_nor_init: power up MTD device");
+ DEBUG_PUTS("mtd_spi_nor_init: power up MTD device");
if (mtd_spi_nor_power(mtd, MTD_POWER_UP)) {
- DEBUG("mtd_spi_nor_init: failed to power up MTD device");
+ DEBUG_PUTS("mtd_spi_nor_init: failed to power up MTD device");
return -EIO;
}
@@ -703,6 +723,9 @@ static int mtd_spi_nor_init(mtd_dev_t *mtd)
mtd->sector_count = mtd_spi_nor_get_size(&dev->jedec_id)
/ (mtd->pages_per_sector * mtd->page_size);
}
+ /* SPI NOR is byte addressable; instances don't need to configure that */
+ assert(mtd->write_size <= 1);
+ mtd->write_size = 1;
_set_addr_width(mtd);
/* verify configuration */
@@ -795,45 +818,6 @@ static int mtd_spi_nor_read(mtd_dev_t *mtd, void *dest, uint32_t addr, uint32_t
return 0;
}
-static int mtd_spi_nor_write(mtd_dev_t *mtd, const void *src, uint32_t addr, uint32_t size)
-{
- uint32_t total_size = mtd->page_size * mtd->pages_per_sector * mtd->sector_count;
-
- DEBUG("mtd_spi_nor_write: %p, %p, 0x%" PRIx32 ", 0x%" PRIx32 "\n",
- (void *)mtd, src, addr, size);
- if (size == 0) {
- return 0;
- }
- const mtd_spi_nor_t *dev = (mtd_spi_nor_t *)mtd;
- if (size > mtd->page_size) {
- DEBUG("mtd_spi_nor_write: ERR: page program >1 page (%" PRIu32 ")!\n", mtd->page_size);
- return -EOVERFLOW;
- }
- if (dev->page_addr_mask &&
- ((addr & dev->page_addr_mask) != ((addr + size - 1) & dev->page_addr_mask))) {
- DEBUG("mtd_spi_nor_write: ERR: page program spans page boundary!\n");
- return -EOVERFLOW;
- }
- if (addr + size > total_size) {
- return -EOVERFLOW;
- }
-
- mtd_spi_acquire(dev);
-
- /* write enable */
- mtd_spi_cmd(dev, dev->params->opcode->wren);
-
- /* Page program */
- mtd_spi_cmd_addr_write(dev, dev->params->opcode->page_program, addr, src, size);
-
- /* waiting for the command to complete before returning */
- wait_for_write_complete(dev, 0);
-
- mtd_spi_release(dev);
-
- return 0;
-}
-
static int mtd_spi_nor_write_page(mtd_dev_t *mtd, const void *src, uint32_t page, uint32_t offset,
uint32_t size)
{
@@ -942,7 +926,6 @@ static int mtd_spi_nor_erase(mtd_dev_t *mtd, uint32_t addr, uint32_t size)
const mtd_desc_t mtd_spi_nor_driver = {
.init = mtd_spi_nor_init,
.read = mtd_spi_nor_read,
- .write = mtd_spi_nor_write,
.write_page = mtd_spi_nor_write_page,
.erase = mtd_spi_nor_erase,
.power = mtd_spi_nor_power,Patch for mtd_spi_nor.hdiff --git a/drivers/include/mtd_spi_nor.h b/drivers/include/mtd_spi_nor.h
index 81b1cd9412..5b97b5626c 100644
--- a/drivers/include/mtd_spi_nor.h
+++ b/drivers/include/mtd_spi_nor.h
@@ -134,8 +134,6 @@ typedef struct {
mtd_jedec_id_t jedec_id; /**< JEDEC ID of the chip */
- uint8_t addr_width; /**< Number of bytes in addresses, usually 3 for small devices */
-
/**
* @brief bitmask to corresponding to the page address
*However, as this PR moves the timing information from the Unfortunately I have none of these boards, so I couldn't test whether the SFDP feature would work here, but moving the timing parameters from one structure to another and setting the I could create patches for the boards as well, but it would be good to get some feedback about what's the best approach before that. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contribution description
JEDEC has a standardized device information tables for serial flash chips. This PR adds limited support for reading these tables and configure the flash parameters based on these tables. The number of address bytes, the chip size, and supported erase sizes with wait times are discovered from the flash.
A flag is added to disable this to allow for manual configuration.
A downside of this change is that most SPI NOR flash parameters have to reside in the MCU memory.
Testing procedure
This can be tested using
examples/filesystem, the board should behave as before this PR, except that less compile time configuration is required.Some SPI NOR flash chips, such as Winbond chips, do not support this. I've added a flag so that manual configuration is still possible. This is also the case for the Mulle board.
Issues/PRs references
None.