Add save states, Memories, and Sleep support#113
Conversation
|
Thank you very much for working on this. I am curious how much of this has been managed by a human vs a LLM. However, I cannot merge or even begin to review this as-is. You have collapsed years of upstream changes into a single commit, which I cannot check for provenance or verify your other, Pocket specific changes. I would like to hook up the same system I built for automatic sync (and potentially directory rewriting) from https://github.com/agg23/openfpga-NES, but I haven't had a chance. There should not be binaries in the repo (the bitstream/ If the only actual changes are in the chip32 and |
Thanks for the quick look, and fair points. I've restructured the branch to address all three. Human vs LLM: this was human-led and hardware-verified. The save state work (base system plus SA-1, GSU/SuperFX, DSP, CX4, SDD1, SPC7110, and PAL timing) was tested against real-core behavior, and the tricky bits (like the GSU restore-settle fix that was wedging Super FX titles) came out of that testing. I used an LLM as an assistant for mechanical and repetitive work, not for the design or the fixes. I also used LLM to help me writing this comment 🤣 The upstream sync is mechanical, not an opaque squash. The History is now separated by concern, so the review surface is the Pocket-specific work rather than the 7.5k-line sync:
No bitstreams in the PR. I removed the bitstream regeneration entirely, so the branch doesn't touch any Let me know if you'd rather I drop the sync commit now and target it at your sync tooling instead. |
|
SS_NMI_FORCE fix is something NOT PRESENT upstream (yet). On Pocket the fix is MUCH MORE important:
|
|
I'm rebasing it with on top of your new master branch. |
f37c880 to
eb268f5
Compare
|
I rebased the patches with the new dual-port SDRAM implementation (so that a sdram.patch is not necessary). |
|
I'm testing a little fix for a RARE S-CPU crash on SA-1 (if you save EXACTLY when SA-1 is calculating stuff). |
|
Sorry to make this more complicated, but for clarity's sake can you remove the NMI patch from this PR? Once SS is merged, we can follow up by applying the NMI patch, keeping everything separate and easier to review. |
ok, I'm studying another approach to remove also the patches for SA1 (by using PSRAM instead of using SDRAM + freeze)... I can't ensure it works, but if it works it should be 100% upstream compatible |
|
It depends on why the patches are necessary and they'd have to be justified. I don't see why the Pocket would be any different than another platform in this regard; the only reason I could foresee is if you are required to substitute memories (for example, if the MiSTer core is using DDR). I do note that there is a lot of LLM "babble" everywhere explaining what it is doing. While comments are appreciated, I would prefer if the owning human explained in their own, correct words why a decision was made. A human would not write I do greatly appreciate your dedication here. I know my requirements can seem difficult and not worth dealing with. |
|
sorry for the "noisy" LLM comments/documents, as an old-style (and old, sigh) developer I sux in writing documentation/comments and this is there I use LLM to help me. I'll remove README.md and add a couple of lines in patches instead (so we/you remember why a patch is needed more easily by just opening it) |
bf9c8b4 to
05ea770
Compare
|
I'm doing the last tests and code optimization, but it looks quite solid! |
1cc8ae2 to
8e054a6
Compare
|
The previous implementation worked, but I preferred to do a smaller (again) implementation by avoid using the PSRAM arbiter, but by moving ARAM to the unused SRAM. Final cleanup/tests and I update the PR for the last time (unless you have other comments) |
|
Done. The save-state blob lives on its own PSRAM chip as a single client. ARAM sits on the Pocket's async SRAM, and WRAM uses the upstream wiring. Keeping the blob on a dedicated chip means state access never touches the live memory paths, so nothing pauses or freezes during a save or load. A dedicated memory is needed because of the Pocket's layout. Upstream MiSTer keeps the state in DDR, which the Pocket doesn't have. The Pocket's SDRAM has plenty of room, but it carries the live console ROM fetches, so keeping the state there makes the transfers collide with ROM reads and forces the core to pause. On-chip BRAM is too small to hold the state. Giving the blob its own PSRAM chip keeps it off the ROM path, so nothing has to pause. The only SNES-core changes are two small patches that add the save-state hooks (savestates.sv and main.v); everything else lives in the Pocket wrapper, so it stays close to upstream. boot1.mif is generated at build time from the copied upstream boot1.rom, so it stays in sync. Tested on hardware: SMW, F-Zero, and SMRPG (including saving mid-combat). I also recorded the MDFourier test signal and compared it against your last released core; the two match to within 0.04 dB on average, so the sound is unchanged (yes, I'm paranoid, sometimes). As SRAM implementation, I used yours from https://github.com/agg23/openfpga-litex. |
agg23
left a comment
There was a problem hiding this comment.
This is a lot to review, so I imagine it will take a few passes from both of us to catch everything.
As you see, I really don't like having dumb LLM comments. I mostly stopped commenting on them after a while. Please remove ALL of them and manually (human) write the ones you believe are necessary.
I spent an hour on this review so far, and I barely got into the actual logic changes. Please please please reign in the LLM; I will not accept code that does not look and act like a human wrote it; a comment in LLM style, code not following the styling of the rest of the project, or obviously less than optimal logic will be flagged by me.
The engine needs hooks in main.v, mirrored from the upstream SNES core, so they ride the existing patch mechanism under .github/upstream_patches/. - main.v: expose the walk status (SS_BUSY) as an output so the platform can tell when a save state walk is running, and turn the chip-enable defines into module parameters so the build can drop save states per variant.
Wraps the upstream save state engine in a mister_top shim, plus the blob transport, the boot1 helper, the chip32 loader and the core.json / generate.tcl / support changes. The engine hijacks an NMI/IRQ vector fetch and runs boot1 on the emulated 65C816 to capture or restore machine state. MiSTer streams the blob to HPS DDR3; the Pocket has no spare memory there, so the blob gets its own PSRAM die (cram1) driven by psram_blob as a single client, and never pauses the game. save_state_controller sequences the APF commands and reports an error when the engine does not fire within the timeout, since a game running with interrupts off never fetches a vector to hijack. The bitstreams split into GSU and SA-1/CX4 variants because the save state register muxes do not fit alongside every chip at once.
generate.tcl split the bitstream into GSU-only and SA-1/CX4 variants (ntsc_sa1cx4, pal_sa1cx4), but the build matrix never referenced them, so they were never built and the stale sa1gsu/pal_sa1gsu map_value cases resolved to build types generate.tcl no longer accepts.
agg23
left a comment
There was a problem hiding this comment.
Thank you for the changes. The diff feels much nicer to me now.
I left a number of comments that don't require you to do anything unless you really want to. The only thing left is the actual savestate state machine work, which to me seems massively over complicated. Feel free to tell me I'm wrong and why the complexity is warranted.
|
|
||
| generate | ||
| if (USE_SS == 1'b1) begin | ||
| data_loader #( |
There was a problem hiding this comment.
I was going to say that instantiating this module again is a waste of LUTs (they tend to take ~200 for some reason, which isn't nothing), but apparently I'm stupid and instantiate data_loader all over the place in my projects (including in this one), so never mind.
Only change it if you really want to
|
|
||
| `timescale 1ns / 1ps | ||
|
|
||
| module tb_psram_model #( |
There was a problem hiding this comment.
What is this sourced from? I don't particularly care about the TBs (and really don't trust a LLM to write hardware tests), but I am curious why the LLM thinks this fully encapsulates the PSRAM spec (it might; it just doesn't look like it to me at a glance)
There was a problem hiding this comment.
I only asked LLM to make some unit tests in order to be easier to get regressions (usually LLM are good for this task) and a couple of times it saved me to build a broken build. if you don't like that, I can remove it
| case (eng_state) | ||
| ENG_IDLE: begin | ||
| if (req_pending) begin | ||
| req_pending <= 0; |
There was a problem hiding this comment.
Is something being accomplished by the request being buffered? A few lines above, you detect the CHANGE in ss_ddr_req (is this intentional? Why isn't this the rising edge?), register all of the inputs, then on the next cycle we are in ENG_IDLE, actually execute the request. This delays by one cycle.
You would only do this if you think requests will come in AND disappear in less than the time it takes for the state machine to get back to ENG_IDLE. If that is the case, that implies the source of the request does not wait for requests to be acked, and thus you could have a dropped request due to them coming in too quickly.
There was a problem hiding this comment.
ss_ddr_req is a toggle, not a level (https://github.com/MiSTer-devel/SNES_MiSTer/blob/master/rtl/savestates.sv#L158).
the buffering is for the prefetch: the engine gets its ack before the prefetch of the next qword finishes, and it only issues when req==ack, so its next request can land while the FSM is still in ENG_WAIT_PF
| cmd_data <= req_data_r; | ||
| cmd_be <= req_be_r; | ||
| cmd_we <= 1; | ||
| cmd_go <= ~cmd_go; |
There was a problem hiding this comment.
The more I read this, the more it looks very strange to me. Why is the cmd_go toggle needed at all? Why are the control signals not sufficient?
There was a problem hiding this comment.
cmd_go is not an additional savestate control signal. it is the request token for the clk_sys -> clk_mem CDC handshake. If you prefer, I can rename it as cmd_req_toggle to be clearer
|
|
||
| // Queue data_loader write bursts so no pulse is dropped; one M10K pair, | ||
| // deep enough to ride out a cart download still winding down | ||
| reg [35:0] stage_fifo[512]; |
There was a problem hiding this comment.
What in the world. Why do you need a MASSIVE FIFO?
There was a problem hiding this comment.
There are also platform primitives for FIFOs
There was a problem hiding this comment.
this was a leftover of the previous SDRAM implementation that needed it. with the new implementation it's not necessary anymore (replaced by a single stage register). thank you for noticing that
| @@ -1,17 +1,28 @@ | |||
| module save_state_controller ( | |||
| // APF Memories command sequencer for savestates.sv. | |||
There was a problem hiding this comment.
It's unfortunate that Paul didn't keep the Robert style savestate system (which I also use in my cores). However, I'm not sure that what Paul did is anywhere near as different as what this diff suggests. Between this file (idk why it was in this project, but it should be a copy from what is in NES), and the latest version in Tamagotchi (https://github.com/agg23/fpga-tamagotchi/blob/4127128ef0495101df09ff004e4ec03899f738ef/target/shared/savestate_machine.sv), I expect there to be minimal changes from one of those versions.
The entire savestate IO system in SNES comes down to the nets:
input [63:0] ddr_di,
output reg [63:0] ddr_do,
input ddr_ack,
output [21:3] ddr_addr,
output reg ddr_we,
output reg [7:0] ddr_be,
output reg ddr_req,This is a very standard memory layout, and should map 1:1 with your storage memory (more on that in a moment). So the only state machine you really need is to manage the simple request to "DDR" and probably to send a wait signal back up to the savestate system so you can control delays necessary to properly funnel the data.
For the same reason, I'm not sure that more physical memory is needed at all. What is preventing you from streaming the savestate from the SNES core savestate control system directly out over the APF bridge? You have full control of the system and it is in a psuedo halted state, so I don't know why waiting the 100 74.25MHz cycles for each word of savestate data would be a problem. This is overall faster than separately writing to a memory.
There was a problem hiding this comment.
The difference is exactly the Robert/Paul distinction you mention.
NES/Tamagotchi halt the core and the fabric walks a directly addressed state bus, so their controller can advance only when its FIFO is ready.
Paul’s engine instead hijacks a NMI/IRQ vector and has an injected program walk the state using CPU accesses and SNES DMA (this is the reason of my upstream PR #488).
In my previous SS implementation (the SDRAM one), I tried to freeze SNES during savestates (it was necessary to use SDRAM for both SNES stuff and the backup itself), but I had lots of problem (especially in the special chipsets, like SA-1). Maybe it's the reason he avoided doing that (SNES MiSTer core does not support pause in OSD too).
ddr_req/ddr_ack serialize access to the backing memory, but they are not backpressure for the CPU/DMA walk. The helper polls SS_STATUS.BUSY once after resetting SSADDR, then performs the bulk load without another status poll. The save path does not poll it either. If the next qword is late, the injected program does not stop. I tried to remove my read-ahead and it makes loading fail on hardware (black screen or weird sprites). We could add word-by-word backpressure, but this would require changing Paul’s injected program/DMA flow, not merely adapting the Pocket controller. As you can see, I kept exactly the same boot1.rom distributed upstream (converted as mif, but not modified in any way).
Fixes #59.
In the issue you noted you were waiting on the
paulb-nlsavestates fork to stabilize and land in the main upstream MiSTer core before integrating it. That has now happened, so this brings the work over to the Pocket.This syncs the core to SNES_MiSTer (2026-06-03), which includes the merged savestates, and wires up save states, Memories, and Sleep support on the Pocket side.
Changes
Notes
Save states cover the base system and the enhancement chips (SA-1, GSU/SuperFX, DSP, CX4, SDD1, SPC7110), along with PAL timing. States have been tested across those variants against real-core behavior.