fix(jtframe): break the MiSTer SDRAM A[12:11] ACTIVATE timing path

[strygo]

Why

jtframe_sdram64 evaluates a 4-bit next_cmd==CMD_ACTIVE compare after the 7-deep grant mux. On MiSTer that term selects SDRAM_A[12:11], which double as DQMH/L on the shorted-pin 128MB module, so bank FSM → grant mux → compare → A[12:11] becomes the longest cone in the design, ending in the DDIO register at the pin (~4 ns of it the single route into the IO ring). Bits 10:0 carry no such term and never violate.

Measured on 2287d18, Quartus Lite 20.1 (the CI image), MiSTer target, seed 1: all 30 of the top 30 setup paths terminate at sdram_a[12:11], and jtcps2 does not close at all — --max-trials is absorbing what is really a structural problem.

core	before	after
jtcps1	+0.087	+0.538
jtcps15	+0.312	+0.349
jtcps2	−0.014	+0.229

With the compare out of series all three close at seed 1, and no sdram_a path remains in the top 30 — the tightest are back to the MiSTer scaler/OSD, where the bottleneck otherwise sits.

What

Each source of the grant mux already knows whether its command is ACTIVATE: the banks decode it as ~do_prech & do_act (exported as act), while init/rfsh/prog compare their own 4-bit cmd off the critical path. next_is_act mirrors the grant mux leaf for leaf and replaces the compare in the registered assignment — same outputs, one cycle, no protocol change, non-MiSTer arm untouched.

This is the same restructuring burst_io received in 29d9b07 ("break MiSTer SDRAM DQM timing path"); jtframe_sdram64 never got it.

Verification

A SIMULATION-gated assertion checks next_is_act == (next_cmd==CMD_ACTIVE) every clock. ver/sdram/sdram_bank64, stock master vs this branch:

config	master	this branch
rw_test, mister 100 MHz	PASS	PASS
rw_test, mister 96 MHz period	PASS	PASS
rw_test, mister 40% write	PASS	PASS
rw_test, non-mister arm	PASS	PASS
prog_test	PASS	PASS
rw_test, mister -idle 10	FAIL — bank 3 stall @ 315155.0 ns	FAIL — bank 3 stall @ 315155.0 ns

The assertion never fired. The -idle 10 failure is pre-existing: it reproduces identically on unmodified master at the same simulation timestamp.

Notes

• The slack figures above were measured on 2287d18; I have not re-measured them on current master.
• Running those suites under Icarus 13 needs the -D flags moved ahead of the source files in ver/sdram/sdram_bank64/*/sim.sh — Icarus 13 only honours defines that precede the sources, so as shipped SDRAM_SHIFT expands to null and the testbench dies at test.v:254. That is pre-existing and unrelated to this change; I ran the suites by invoking iverilog directly with the same sources and defines. Happy to send it as a separate PR if useful.

[jotego]

I don't see this problem in the compilations.

[strygo]

Thanks for approving the workflow run. The three failures look like a fork limitation rather than something in this branch. All three fail at the actions/checkout step, before any source is read:

Secret source: None
##[error]Input required and not supplied: token

framework.yaml, linter.yaml and simunit.yaml all check out with token: ${{ secrets.PAT }}. Secrets aren't exposed to pull_request runs originating from a fork, so that input resolves to an empty string and checkout rejects it. beta_checks passes because it's the one job that doesn't pass an explicit token and falls back to GITHUB_TOKEN. #1506 fails on the same three checks with the same error, so it doesn't appear specific to this PR.

Approving the run unfortunately doesn't change this: approval lets the workflows start, but fork runs still get no secrets. Dropping the token: line wouldn't work either, since submodules: recursive needs credentials for the private submodules.

Happy to do whatever is easiest on your side, whether that's pushing the branch in-repo where secrets are available, splitting it differently, or anything else you'd prefer.

In the meantime I ran the sdram_bank64/rw_test unit sim locally against both this branch and master, in MiSTer mode (-mister -len 0 32 -len 1 16, 25 ms):

	master	this branch
Result	PASSED	PASSED
Performance	58.0%	58.0%
Throughput @ 100MHz	111 MB/s	111 MB/s
Latency ave (b0-b3)	13 / 14 / 20 / 32	13 / 14 / 20 / 32
Latency worst	107 / 30 / 70 / 192	107 / 30 / 70 / 192

Identical figures on both. The change registers the A[12:11] ACTIVATE path without altering the access pattern or costing a cycle.

[jotego]

The answer sounds like coming from AI and it does not reply to my question. I do not see problems in the current code base that will be fixed by this PR so I am not taking it.

[strygo]

@jotego What question did you have? I'm working on a feature that requires a bit more headroom in these cores, and this was the biggest hot spot I could find. With this change, there is more breathing room. I was hoping that since it was consistent with your earlier changes, it would be a reasonable change.

[jotego]

I guess that's the question: what is the need because it is not fixing any current problem.

It also happens that I was trying to reach 128MHz for rungun right now. I tested this on that branch but it wasn't a clear cut. I can give more details later.

Take into account that you should at least do four builds and average the STA results before assuming an improvement because timing easily changes by plus minus 0.3ns between builds.

Maybe we can do this later when merging your other feature?

[jotego]

Validation passed:

simunit-all.sh --only sdram
PASS

STA results, CPS3 MiSTer --nodbg, 4 builds each:

Set	Seeds	Avg WNS	Avg TNS	Best WNS	Worst WNS
Baseline	29778, 1770, 17718, 30916	-0.891 ns	-30.905	-0.453	-1.230
Patched	980, 10891, 21243, 3680	-0.730 ns	-21.870	-0.370	-1.524

Delta: +0.161 ns average WNS, +9.035 average TNS.

Worst paths from TimeQuest are still in the emu|pll...general[4] 112 MHz clock domain, but the actual top path is not a PLL path and not SDRAM_A[12:11]: it is jtframe_mister_dwnld.ioctl_addr[*] -> jtframe_dwnld.prog_addr[*]. The PLL string is just the clock name.

the patch is mildly positive on average, but not a strong CPS3 win. One patched seed is worse than any baseline seed, so I would not justify it from CPS3 STA alone. It is only worth applying if we specifically want the MiSTer SDRAM A[12:11] ACTIVATE path cleanup for other compile-all failures.

[jotego]

Tested again on top of commit 2dc023f, which added a -0.5ns improvement on CPS3 core:

CPS3 MiSTer --nodbg 4-build STA came out as:

seed   WNS       TNS
8132   -0.153   -0.558
21435  -0.220   -1.625
9697   -0.359   -11.559
21833  -0.362   -16.159

Avg WNS: -0.274 ns
Avg TNS: -7.475 ns

Compared with the baseline:

baseline Avg WNS: -0.363 ns
baseline Avg TNS: -11.597 ns

delta WNS: +0.090 ns
delta TNS: +4.122 ns

The WNS improvement is modest and right around the usual fitter-noise boundary. The remaining top violations are now CPS3 cache RAM/tag paths, not the SDRAM A[12:11]/mask-selection cone targeted by the PR.

[jotego]

The S18 core is currently not passing STA so this one was good test.

s18, 12 builds

baseline passes: 4 / 12
patched  passes: 3 / 12

baseline Avg WNS: -0.183 ns
patched  Avg WNS: -0.275 ns
delta:            -0.092 ns

baseline Avg TNS: -0.980 ns
patched  Avg TNS: -1.723 ns
delta:            -0.743 ns

baseline median WNS: -0.131 ns
patched  median WNS: -0.221 ns
delta:              -0.091 ns

The PR patch is worse on average WNS, average TNS, median WNS, and pass count.

Definetely this patch is not worth it.