Add FlashRAM save storage driver#925
Conversation
New flashram.c/.h module for the 128 KiB (1 Mibit) Macronix-family FlashRAM save chip found in some N64 cartridges. Unlike SRAM it is command-driven (identify / read / status / sector-erase / page-program) rather than flat memory; the protocol matches real hardware and is compatible with ares and SC64. Two API levels: a low-level page/sector interface, and a high-level flat byte-range flashram_read/flashram_write that behaves like SRAM. flashram_write does a per-sector read-modify-write so a partial-sector write preserves the untouched bytes in that 16 KiB erase sector. The driver manages its own PI-DMA cache coherency.
rasky
left a comment
rasky
left a comment
There was a problem hiding this comment.
First round mainly focusing on public API, and just a quick look at implementation
| #define FLASHRAM_PAGE_SIZE 0x00000080 ///< Program granularity (128 bytes). | ||
| #define FLASHRAM_SECTOR_SIZE 0x00004000 ///< Erase granularity (16 KiB). | ||
| #define FLASHRAM_NUM_PAGES (FLASHRAM_SIZE / FLASHRAM_PAGE_SIZE) ///< Number of programmable pages (1024). | ||
| #define FLASHRAM_NUM_SECTORS (FLASHRAM_SIZE / FLASHRAM_SECTOR_SIZE) ///< Number of erase sectors (8). |
There was a problem hiding this comment.
API-wise I'd rather be a little more flexible for future homebrew, in case somebody wants to attempt support for flashes with different sizes. Instead of hardcoding these, we should probably have a description structure being returned by flashram_detect() when a flash is detected. I see it returns FLASHRAM_SIZE but that gives not enough information on the layout.
There was a problem hiding this comment.
One possibility that I used during my experiments was to describe flash layout using number of bits per component: https://github.com/bsmiles32/g64drive/blob/feat/ultrasave_wip/ultrasave/flash/layout.go#L10-L45
This could describe both byte and word based flash, and could have been extended to other page/sector sizes.
| * @return The 32-bit status word. The low bits report program/erase busy and | ||
| * completion state (see the implementation for the bit layout). | ||
| */ | ||
| uint32_t flashram_status(void); |
There was a problem hiding this comment.
This is maybe a bit too low-level. The exact layout of the flashram registers is probably better hidden within the module. What's the use case for having this function in the public API?
| * | ||
| * Reads a byte range from FlashRAM into @p dst. Any offset and length are | ||
| * accepted; the read is served entirely via PI DMA (FlashRAM array data cannot | ||
| * be read with CPU I/O). |
There was a problem hiding this comment.
I don't think this is generally true. Maybe this is true for word-sized flashes? Anyway it seems like an implementation detail that shouldn't be part of the API documentation.
There was a problem hiding this comment.
FlashRAM array can be accessed just fine with CPU I/O. There are 2 concerns w.r.t. read array mode: the distinction between word-based or byte-based flash layout (word based layout need address adjustment) and the 256-page boundary crossing (only happens for DMA). See https://n64brew.dev/wiki/Flash for details.
| * @param len Number of bytes to write. | ||
| * @return Number of bytes written, or a negative value on error (sets @c errno). | ||
| */ | ||
| int flashram_write(const void* src, size_t offset, size_t len); |
There was a problem hiding this comment.
I have two main discussion points on this API:
- A blocking API for an operation that takes tens of milliseconds seems rather unfortunate. Multithreading is still a hard sell, if we could design an asynchronous API, we would probably be in a better situation.
- I understand the idea of providing a "higher level API" here but this one seems to be very hard to be successfully used:
- With the default layout, up to 1/8th of the flash would need to be cleared for any write basically
- This means that internally this function has to allocate a buffer of non trivial size for the RMW cycle
- The operation is extremely slow; callers will want to minimize the number of unnecessary sector clears
- There is no context to know whether a sector really needs to be cleared or can just be "appended" by programming more pages. So this API has to assume the worst.
It seems to me this is not really ideal. I doubt there will be many users that will want to cope with these compromises.
| *PI_BSD_DOM2_PWD = 0xc; | ||
| *PI_BSD_DOM2_PGS = 0xd; | ||
| *PI_BSD_DOM2_RLS = 0x2; | ||
| enable_interrupts(); |
There was a problem hiding this comment.
Unfortunately the way PI domains are designed makes them a bit unfortunate to use. Here you're basically assuming that nobody else will need to access any other peripheral in the Domain 2, during the whole run. This might be true and even likely, but it's still a very strict requirements.
In theory we should work towards a better lower-level PI API where we describe peripherals and setup domain parameters for them, and then lazily update PI registers when needed. Anyway this is probably not required to be sorted out now.
| // Restore read mode so a later CPU/DMA read returns array data. | ||
| flashram_command(FLASHRAM_CMD_READ_MODE); | ||
|
|
||
| return (id[0] == FLASHRAM_IDENTIFIER) ? FLASHRAM_SIZE : 0; |
There was a problem hiding this comment.
I think here there should be multiple valid identifiers? Also there's the whole issue of byte-access vs word-access that should probably be handled. I think we can leave it unhandled if you don't want to tackle that at first, but we should then probably document in the public API that only one specific model of flash is supported for now?
| { | ||
| errno = EINVAL; | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
I'm a bit doubtful whether we should use errno here or use assertions. Normally we use assertions in most libdragon APIs that shouldn't fail, and only use errno in the POSIX codepaths.
| // Partial sector: read-modify-write to preserve untouched bytes. | ||
| if (sector_buf == NULL) | ||
| { | ||
| sector_buf = memalign(16, FLASHRAM_SECTOR_SIZE); |
There was a problem hiding this comment.
isn't this a good use case for scratch memory?
There was a problem hiding this comment.
I didn't use scratch memory for this because I wasn't sure if we were going to move forward with the eventual consistency model. If the allocation needs to live longer than the function call, scratch becomes riskier.
For a short-lived allocation in a synchronous function with no other allocations it doesn't really make a difference.
bsmiles32
left a comment
bsmiles32
left a comment
There was a problem hiding this comment.
Thanks for working on this ! Just did a first round of review.
| #define FLASHRAM_PAGE_SIZE 0x00000080 ///< Program granularity (128 bytes). | ||
| #define FLASHRAM_SECTOR_SIZE 0x00004000 ///< Erase granularity (16 KiB). | ||
| #define FLASHRAM_NUM_PAGES (FLASHRAM_SIZE / FLASHRAM_PAGE_SIZE) ///< Number of programmable pages (1024). | ||
| #define FLASHRAM_NUM_SECTORS (FLASHRAM_SIZE / FLASHRAM_SECTOR_SIZE) ///< Number of erase sectors (8). |
There was a problem hiding this comment.
One possibility that I used during my experiments was to describe flash layout using number of bits per component: https://github.com/bsmiles32/g64drive/blob/feat/ultrasave_wip/ultrasave/flash/layout.go#L10-L45
This could describe both byte and word based flash, and could have been extended to other page/sector sizes.
| * | ||
| * @return #FLASHRAM_SIZE if FlashRAM is detected, 0 otherwise. | ||
| */ | ||
| int flashram_detect(void); |
There was a problem hiding this comment.
Should we also return the manufacturer / model or their ID ? and as suggested the flash layout instead of just its size ?
| * @return The 32-bit status word. The low bits report program/erase busy and | ||
| * completion state (see the implementation for the bit layout). | ||
| */ | ||
| uint32_t flashram_status(void); |
There was a problem hiding this comment.
At the hardware level, the status register is only 8bit (which happens to be accessed as 32bit because of CPU I/O). So we may return just an uin8_t here (and hope the compiler doesn't make us pay too much for the uint32_t -> uin8_t cast)
There was a problem hiding this comment.
Also, if you expose a low-level API for the FlashRAM you probably also want to expose a "clear status" function because in case of error, status should be cleared before attempting another program/erase operation.
| * | ||
| * Reads a byte range from FlashRAM into @p dst. Any offset and length are | ||
| * accepted; the read is served entirely via PI DMA (FlashRAM array data cannot | ||
| * be read with CPU I/O). |
There was a problem hiding this comment.
FlashRAM array can be accessed just fine with CPU I/O. There are 2 concerns w.r.t. read array mode: the distinction between word-based or byte-based flash layout (word based layout need address adjustment) and the 256-page boundary crossing (only happens for DMA). See https://n64brew.dev/wiki/Flash for details.
| * @param sector Sector index (0 to #FLASHRAM_NUM_SECTORS - 1). | ||
| * @return true on success, false on timeout/failure (sets @c errno on bad args). | ||
| */ | ||
| bool flashram_erase_sector(unsigned int sector); |
There was a problem hiding this comment.
At the hardware level the erase sector command takes a "page number" not a sector number. Effective sector is deduced from the page number. Do we want to use the same formalism at API level and only expose page numbers (and provide some helper functions to convert / work with sectors) ?
NOTE: I had a similar similar choice to make during my experiments in go and I went with page numbers everywhere and having some helper functions (based on chip layout) to return sector being/end pages and so on. This also led to the use of page ranges (also useful for read array operation) which I decided to encode as [begin,end[. I felt that using page numbers everywhere was matching the hardware better.
|
|
||
| // Enter identify mode and DMA out the two silicon-ID words. | ||
| uint32_t id[2] __attribute__((aligned(16))) = {0}; | ||
| flashram_command(FLASHRAM_CMD_IDENTIFY_MODE); |
There was a problem hiding this comment.
Same as previous comment about quirks on real HW : switching to SiliconID mode may require 2 consecutive writes to CIR to be effective on some chips.
| } | ||
|
|
||
| flashram_command(FLASHRAM_CMD_READ_MODE); | ||
|
|
There was a problem hiding this comment.
This only works on byte-based flash, not word-based one. Maybe add a comment about that.
| unsigned int page = sector * FLASHRAM_PAGES_PER_SECTOR; | ||
| flashram_command(FLASHRAM_CMD_SECTOR_ERASE | page); | ||
| flashram_command(FLASHRAM_CMD_EXECUTE_ERASE); | ||
| return flashram_wait_ready(FLASHRAM_STATUS_ERASE_BUSY, FLASHRAM_STATUS_ERASE_OK, |
There was a problem hiding this comment.
You should probably clear the status register before returning from this function.
| dma_write_raw_async(buffer, FLASHRAM_ADDRESS, FLASHRAM_PAGE_SIZE); | ||
| dma_wait(); | ||
| flashram_command(FLASHRAM_CMD_EXECUTE_PROGRAM | page); | ||
| return flashram_wait_ready(FLASHRAM_STATUS_PROGRAM_BUSY, FLASHRAM_STATUS_PROGRAM_OK, |
There was a problem hiding this comment.
Same comment, you should probably clear status before returning from this function.
| uint64_t start_ms = get_ticks_ms(); | ||
| while (true) | ||
| { | ||
| uint32_t status = flashram_status(); |
There was a problem hiding this comment.
Calling flashram_status will cause a write to CIR to switch to status mode. In the case of erase and program operations, WSM (Write State Machine) will "automatically" switch to status mode, so you don't have to write anything to CIR to poll status register for completion/errors.
|
Not necessarily critical for this PR, but I wanted to capture the suggestion from Discord to add an example that leverages this feature. |
Make the FlashRAM driver widely compatible with the different Macronix / Matsushita parts a physical cart might ship, rather than assuming the one byte-indexed layout emulators present: - Detect the chip. flashram_detect() now fills a flashram_info_t (silicon ID, manufacturer/device, model name, addressing mode, layout) and caches the byte- vs word-indexed convention looked up from a model table. The addressing mode cannot be probed at runtime, only identified. - Support word-indexed parts (e.g. MX29L1100): the read path halves the logical offset to reach the right 16-bit word. Both modes share the same 0x8000 logical-byte DMA no-cross boundary (word-mode's 0x4000 PI-address boundary maps to 0x8000 logical), so PGS is set to max (never auto-split) and every DMA is split manually. - CIR double-write for status / silicon-ID mode entry (MX29L1100 needs two writes to actually switch); read/erase/program still use a single write. - Status is 8-bit now (mask off the upper bits, which are garbage from the previous mode on MX29L1100). Add flashram_clear_status() and reset the OK/error latch after every erase/program so a prior result cannot mask the next. The busy-poll reads the auto-latched status directly instead of re-commanding status mode each iteration (which can clear the OK bit). - PI DOM2 LAT 0x05 -> 0x40 (FlashRAM needs a longer latch than SRAM). - Rename the 0xB4 command to LOAD_BYTE_PAGE and 0xA5 to PROGRAM_PAGE to match the documented protocol (0xB4 loads the page buffer, 0xA5 programs it); the old FLASHRAM_CMD_PAGE_PROGRAM name pointed at the wrong step. Byte path validated end-to-end on ares (write/RMW/erase/program/read round-trip + untouched-neighbor preservation).
Move the low-level page/sector interface (status, clear_status, erase, program_page) out of the public header into a new src/flashram_internal.h. The public API is now just the SRAM-like byte-range interface (init/detect/read/write) plus the info struct and geometry constants, while the shape of a low-level API is still being decided. Following the hardware formalism, the erase operation is addressed by page number (the chip derives the enclosing sector), so rename it to flashram_erase_sector_at_page() and thread page numbers through the write path. Add flashram_erase_chip() (0x3C setup + 0x78 execute) to the internal API for protocol completeness.
A save-backed doodle canvas that probes the hardware to discover which save chip is present (EEPROM, SRAM, or FlashRAM) and drives all three through a uniform read/write path, persisting the drawing across power cycles. Built into one ROM per save type from a single source. Probe order is EEPROM -> SRAM -> FlashRAM: FlashRAM detection writes to the command register at +0x10000, which aliases into a 32 KiB SRAM cart's window, so FlashRAM is only probed once SRAM is ruled out. The cursor moves via D-Pad (discrete cells) or the analog stick (continuous, deflection-proportional for precise control), and drawing is done with the RDP via rdpq.
Evolve the FlashRAM public API following PR review: - Describe the chip with a bits-per-component flashram_layout_t (unit/offset/page/sector/read-page bits), replacing the byte/word addressing enum. It uniformly covers byte- and word-indexed parts and encodes the DMA read boundary, so both the addressing shift and the 0x8000 boundary are now derived rather than hardcoded. - Base every read/write/erase/program on the detected layout cached in flashram_info_t rather than the fixed geometry macros (which now only seed the pre-detection default and bound the page-program buffer). - Merge detection into flashram_init(): it configures the PI timings, probes the silicon ID, caches the layout, and returns presence (optionally filling flashram_info_t). flashram_detect() is removed -- the layout must be known for correct addressing, so there is no useful init-without-detect state. - Let flashram_init() take optional PI DOM2 timings (NULL = the standard wiki defaults: latency 0x05, pulse 0x0C, page size 0x0F, release 0x02). - Assert a chip was actually detected before any read/write, drop the now-unused FLASHRAM_PAGES_PER_SECTOR, and stop referencing the internal API from the public header docs.
Detect FlashRAM via the merged flashram_init(NULL, &info), which now returns presence and fills the info in one call.
The PI_BSD_DOM2 LAT/PWD/PGS/RLS registers are a generic PI-domain concept, not FlashRAM-specific, so move the timings struct into dma.h (libdragon's PI interface) as pi_dom_timings_t and have flashram_init() take it. The page-size-must-be-0x0F requirement is documented on flashram_init(), where it belongs, rather than on the generic type.
The chip geometry is now detected and reported per chip in flashram_info_t, so the public FLASHRAM_SIZE / PAGE_SIZE / SECTOR_SIZE / NUM_PAGES / NUM_SECTORS macros were redundant (and misleading, implying a fixed layout). Remove them, keeping only FLASHRAM_ADDRESS. The page-program buffer's compile-time bound becomes an internal FLASHRAM_MAX_PAGE_SIZE, and __flashram_info is left zero-initialized since reads/writes assert a chip was detected before reading it.
The new savedoodle example is very cool! Nice work constructing an elegant way to exercise the various save types. |
FLASHRAM_LAYOUT_BYTE/WORD lacked doc comments, failing the -Werror Doxygen build. Give each a one-line description of the geometry it encodes.
Sector-erase/page-program commands OR the page number into a 32-bit opcode word (opcode in bits [31:24]), so page numbers must fit in the low 24 bits. The per-op range checks only compare against num_pages, so a table entry with an oversized geometry would pass yet silently corrupt the opcode. Assert num_pages <= 2^24 in flashram_derive_geometry, where the geometry is adopted, turning the implicit invariant into a checked one at no hot-path cost.
4 participants
New flashram.c/.h module for the 128 KiB (1 Mibit) Macronix-family FlashRAM save chip found in some N64 cartridges. Unlike SRAM it is command-driven (identify / read / status / sector-erase / page-program) rather than flat memory; the protocol matches real hardware and is compatible with ares and SC64.
Two API levels: a low-level page/sector interface, and a high-level flat byte-range flashram_read/flashram_write that behaves like SRAM. flashram_write does a per-sector read-modify-write so a partial-sector write preserves the untouched bytes in that 16 KiB erase sector. The driver manages its own PI-DMA cache coherency.