AP_Filesystem: add LittleFS support for Micron MT29Fxx SPI NAND#32146
AP_Filesystem: add LittleFS support for Micron MT29Fxx SPI NAND#32146andyp1per wants to merge 2 commits intoArduPilot:masterfrom
Conversation
4b4e499 to
0cad205
Compare
Add WSPI driver for the Micron MT29F SPI NAND flash family, enabling LittleFS filesystem on boards with QUADSPI-connected NAND flash. Supported devices: MT29F1G/2G/4G/8G (1-8Gbit densities) Features: - Quad I/O mode (1-1-4) for both read and write data transfers - Internal ECC enabled - Reversed JEDEC ID byte order handling
Support 'littlefs:mt29fxx' in DATAFLASH directive for WSPI-connected Micron MT29F SPI NAND flash chips.
0cad205 to
2114348
Compare
There was a problem hiding this comment.
I don't understand why this change is so large and complex. The commands are all the same as W25Nxx (except two little things) and the existing driver could have been reused with a two line change if the chip was wired to an existing SPI port.
Do you have an example hwdef? Can the board use a normal SPI port? I wouldn't think QSPI would provide a noticeable advantage, program times are high and the system is bottlenecked on USB on read.
Assuming QSPI is advantageous, there should just need to be a few defines in the transfer functions to switch between using the SPI object and the WSPI object. Then also W25Nxx would get WSPI support.
Peter is probably right that it's time to bring the flash driver out into a separate class though, LittleFS already has a clean separation. We only compile support for one flash driver in anyway so I don't understand the size concern.
| // used by LittleFS | ||
| #define AP_FILESYSTEM_FLASH_JEDEC_NOR 1 | ||
| #define AP_FILESYSTEM_FLASH_W25NXX 2 | ||
| #define AP_FILESYSTEM_FLASH_WSPI_NAND 3 |
There was a problem hiding this comment.
Is this generic or specific to MT29FXX?
| #endif | ||
|
|
||
| //#define AP_LFS_DEBUG | ||
| // Uncomment to enable debug output for WSPI NAND development |
There was a problem hiding this comment.
This comment is nonsense.
| #define SPINAND_CMD_RESET 0xFF | ||
| #define SPINAND_CMD_GET_FEATURE 0x0F | ||
| #define SPINAND_CMD_SET_FEATURE 0x1F | ||
| #define SPINAND_CMD_WRITE_ENABLE 0x06 | ||
| #define SPINAND_CMD_PAGE_READ 0x13 | ||
| #define SPINAND_CMD_READ_FROM_CACHE 0x03 // 1-1-1 mode | ||
| #define SPINAND_CMD_READ_FROM_CACHE_X4 0x6B // 1-1-4 mode (quad read) | ||
| #define SPINAND_CMD_PROGRAM_LOAD 0x02 // 1-1-1 mode | ||
| #define SPINAND_CMD_PROGRAM_LOAD_X4 0x32 // 1-1-4 mode (quad write) | ||
| #define SPINAND_CMD_PROGRAM_EXECUTE 0x10 | ||
| #define SPINAND_CMD_BLOCK_ERASE 0xD8 | ||
| #define SPINAND_CMD_READ_ID 0x9F | ||
|
|
||
| /* SPI NAND standard register addresses */ | ||
| #define SPINAND_REG_PROTECTION 0xA0 | ||
| #define SPINAND_REG_CONFIG 0xB0 | ||
| #define SPINAND_REG_STATUS 0xC0 | ||
|
|
||
| /* SPI NAND standard status register bits */ | ||
| #define SPINAND_STATUS_OIP 0x01 // Operation In Progress | ||
| #define SPINAND_STATUS_WEL 0x02 // Write Enable Latch | ||
| #define SPINAND_STATUS_EFAIL 0x04 // Erase Fail | ||
| #define SPINAND_STATUS_PFAIL 0x08 // Program Fail | ||
|
|
||
| /* SPI NAND standard config register bits */ | ||
| #define SPINAND_CONFIG_ECC_ENABLE (1 << 4) | ||
|
|
||
| /* MT29FXX timing */ | ||
| #define SPINAND_TIMEOUT_RESET_MS 2 // tRST max | ||
| #define SPINAND_TIMEOUT_PAGE_READ_US 70 // tRD max (ECC enabled) | ||
| #define SPINAND_TIMEOUT_PAGE_PROGRAM_US 220 // tPROG typ (ECC enabled) | ||
| #define SPINAND_TIMEOUT_BLOCK_ERASE_MS 10 // tBERS max | ||
|
|
||
| /* MT29FXX geometry - common to all densities */ | ||
| #define SPINAND_PAGE_SIZE 2048 | ||
| #define SPINAND_BLOCK_SIZE (64 * SPINAND_PAGE_SIZE) // 128KB |
There was a problem hiding this comment.
These, with the exception of timing and GET_FEATURE/SET_FEATURE, are the same as JEDEC and W25Nxx. We don't need to redefine all of them.
W25Nxx also takes 0x0F as an alias for READ_STATUS and 0x1F as an alias for WRITE_STATUS. So we could just use those opcodes on NAND flash. They don't appear supported on NOR devices.
| # WSPI-connected NAND flash (Micron MT29F family: 1G/2G/4G/8G) | ||
| f.write('#define AP_FILESYSTEM_LITTLEFS_FLASH_TYPE AP_FILESYSTEM_FLASH_WSPI_NAND\n') | ||
| f.write('#define AP_FILESYSTEM_LITTLEFS_MT29FXX_ENABLED 1\n') | ||
| # Only disable FATFS if there's no SDMMC or SPI-connected SD card |
There was a problem hiding this comment.
Huh? We can't support this. We do need to disable FATFS unconditionally.
Its only larger because this is supporting QSPI. The board I have only supports QSPI. Really this is about adding QSPI support to littlefs rather than a specific chip, so perhaps the names need changing to suit.
Yes. And no it can't use SPI because the pins are different. The relevant bits look like this:
Its not as easy as that. We are leveraging STM32's QSPI support (in the same way that we do for external flash) and it is that support that dictates the way the functions need to operate.
That was my initial thought but it actually makes the PR much bigger and more complicated. We also have the AP_FlashIFace stuff which at some point should be factored to support this, but it only currently supports the model supported by NOR flash - so again a big refactor. The current changes have no size impact if not used and so I have concluded that refactoring here is more trouble than its worth. |
|
Thanks for confirming the pins are different.
Here I meant a hypothetical board with W25Nxx hooked up to the QSPI pins. I still see the operation as basically the same, it should just be functions that touch |
|
I'm happy to see support for this, but I'll defer to @tpwrules and @andyp1per to sort out the details, both of you know far more than I do about this stuff |
peterbarker
left a comment
There was a problem hiding this comment.
I'm quite uncomfortable with extending this for wspi support.
We are duplicating more code - not just within this file as Thomas has pointed out, but we also have qspi support already in AP_FlashIface, the interface I think everybody thinks we should move to for LittleFS, bootloader and logging support.
At some stage we have to stop and clean things up, and I think this might be that point.
| AP_HAL::Device::CommandHeader hdr{}; | ||
| hdr.cmd = cmd; | ||
| hdr.cfg = AP_HAL::WSPI::CFG_CMD_MODE_ONE_LINE | | ||
| AP_HAL::WSPI::CFG_ADDR_MODE_ONE_LINE | | ||
| AP_HAL::WSPI::CFG_ADDR_SIZE_24; | ||
| hdr.addr = addr; |
There was a problem hiding this comment.
Please initialize these as a ```
const AP_HAL::Device::Commandhdr {
.
.
.
};
Summary
littlefs:mt29fDATAFLASH directive support in the hwdef build script for WSPI-connected NAND flashTest plan