Fix Verilog-2001 violation: enforce standard-compliant inout handling.

[jefchaves]

Fixing a Verilog-2001 standard violation that breaks simulation, synthesis, and formal verification in standards-compliant tools:
Bidirectional ports (inout) must be driven using continuous assignments (assign), which are only valid for nets (e.g., wire). Using inout reg is explicitly disallowed. Tri-state and bidirectional ports must therefore be implemented with continuous assignments on nets. By contrast, reg types are intended for procedural assignments (inside always blocks), which cannot synthesize proper tri-state drivers for I/O pads.

[sorgelig]

Great it compatible with simulators with this change but how about input/output slacks in TimeQuest for SDRAM_DQ after this change?

[jefchaves]

I can try to measure those slacks. I synthesized the circuit with my changes using Quartus Prime 17.1.0 Build 590 10/25/2017 SJ Standard Edition. Which is the official version of Quartus used to build the released cores?

[sorgelig]

It's still not obvious that fitter will place data into IOBUF which was the original goal of the code. Simulator compatibility is not the priority.

[jefchaves]

Thank you for pointing that out. I understand that placing data into the IOBUF is a critical concern. I checked both synthesized circuits using Quartus’ Technology Map Viewer (Post-Fitting). Here are my conclusions:

In the original code, a register (SRAM_DQ~reg) was inferred, placed between the data and SRAM_DQ. Please see FIG. 1.

Line 205 in commit 35ea23d produces a combinational circuit, essentially just a wire, connecting the data signal to the IOBUF. Please see FIG. 2.

The data flow is as follows:

1. In the original circuit:

"SDRAM_DQ~input" feeds "data_reg" (FIG. 3)

"data" feeds "SDRAM_DQ~reg" (FIG. 1)
"SDRAM_DQ~reg" feeds "SDRAM_DQ~output" (FIG. 1)

2. In the proposed circuit (commit 35ea23d):

"SDRAM_DQ~input" feeds "data_reg" (FIG. 2)

"data" feeds "SDRAM_DQ~output" (FIG. 2)

In both cases, the filter controlling the IOBUF was determined by the same option in the same case statement. In the original code, Quartus inferred a register called SRAM_DQ~en, whereas in the proposed commit, I explicitly created SDRAM_DQ_CONTROL. FIG. 4 and 5 show that both have the same sources, which was expected given the source codes.

Therefore, the proposed commit preserves data flow, reduces delay (by eliminating one register in the data flow), and eliminates language semantics violations (Verilog-2001).

MiSTer FPGA is undoubtedly an incredible project — thank you very much for that. I understand that it can be complicated to change things that already work, but my point here is to enable MiSTer to benefit from compatibility with other tools, especially formal verification tools. There are impressive open-source tools for verification. This small change could bring significant benefits to the project in the future, such as addressing potential issues more quickly, optimizing the circuit, and reducing size, delays, and complexity.

[sorgelig]

DQ~reg was inferred obviously because it's explicitly written in the code. It doesn't produce additional delay between CLK and DQ. Because 'data' is additionally registered into DQ

NES_MiSTer/rtl/sdram.sv

Line 213 in bfc4386

{2'b11, MODE_NORMAL, STATE_CONT }: {SDRAM_nRAS, SDRAM_nCAS, SDRAM_nWE, SDRAM_DQ} <= {CMD_WRITE, data};

it gives some room for fitter, so it has a flexibility in 'data' register placement as it's not output register. It's positively affects Fmax. In your case, fitter has to place 'data' into IOBUF, so other logic sending data to 'data' must be closer to IOBUF which may negatively impact the placement.

It's not that your change is exactly bad. It's not. But looking how NES core became from simple to very complicated because of many mappers, i have to think how it may affect the work. Sometimes it's not immediate affect but may give problem later. I will think.

[jefchaves]

Thank you for considering that. I understand and agree with you about these concerns.

Best regards