gen: use sys_clk_freq for SDRAMPHYModel timings, instead of 100MHz.

jfng commented 3 years ago

Currently, litedram.gen --sim ignores core_config["sys_clk_freq"] and uses 100MHz for its PHY settings. This PR attempts to make it work with any clock frequency (e.g. 1MHz).

(40d822d has also been tested on the digilent_arty and lambdaconcept_ecpix5 targets).

enjoy-digital commented 3 years ago

Thanks @jfng, the changes related to the simulation model seem fine. I need to have a closer look at https://github.com/enjoy-digital/litedram/pull/266/commits/40d822df63d491289535c584646121fba40e8c04. Can you explain a little bit more the limitation you identified?

jfng commented 3 years ago

Repro

The bug can be reproduced by running litex_sim --with-sdram, after this patch is applied to litex_sim.py:

diff --git a/litex/tools/litex_sim.py b/litex/tools/litex_sim.py
index b1ab167c..d159df67 100755
--- a/litex/tools/litex_sim.py
+++ b/litex/tools/litex_sim.py
@@ -116,17 +116,16 @@ class SimSoC(SoCCore):

         # SDRAM ------------------------------------------------------------------------------------
         if with_sdram:
-            sdram_clk_freq = int(100e6) # FIXME: use 100MHz timings
             if sdram_spd_data is None:
                 sdram_module_cls = getattr(litedram_modules, sdram_module)
                 sdram_rate       = "1:{}".format(sdram_module_nphases[sdram_module_cls.memtype])
-                sdram_module     = sdram_module_cls(sdram_clk_freq, sdram_rate)
+                sdram_module     = sdram_module_cls(sys_clk_freq, sdram_rate)
             else:
-                sdram_module = litedram_modules.SDRAMModule.from_spd_data(sdram_spd_data, sdram_clk_freq)
+                sdram_module = litedram_modules.SDRAMModule.from_spd_data(sdram_spd_data, sys_clk_freq)
             self.submodules.sdrphy = SDRAMPHYModel(
                 module     = sdram_module,
                 data_width = sdram_data_width,
-                clk_freq   = sdram_clk_freq,
+                clk_freq   = sys_clk_freq,
                 verbosity  = sdram_verbosity,
                 init       = sdram_init)
             self.add_sdram("sdram",

Before this PR

When the SDRAMPHYModel is instantiated with clk_freq=1e6 or lower, Wishbone transactions to the user port get stuck until they are aborted. The BankMachine FSM spends its time handling refreshes, and no user commands are able to be sent to the Multiplexer.

litedram_sim_ko

The following loop happens:

A) FSM enters "REGULAR", cmd_buffer.source.valid is high, but the row is closed
B) FSM enters "ACTIVATE", the row is opened
C) FSM enters "REGULAR", cmd_buffer.source.valid is high, but a refresh request takes precedence
D) FSM enters "REFRESH", the row is closed
E) same as A), etc.

The FSM loops over these states, until the Wishbone transaction is aborted.

After this PR

40d822d attempts to guarantee that in this scenario, at least one user command is able to go through.

In C), the next user command will take precedence over a refresh request if it is valid and has not yet been consumed by the Multiplexer (i.e. cmd.valid & ~cmd.ready). Only one user command (if any) is allowed to precede a refresh request, others will have to wait.

litedram_sim_ok

The FSM will still loop a few times, but the Wishbone transaction eventually completes.

enjoy-digital commented 3 years ago

Thanks @jfng, I now see. The refresh indeed has the priority and in the case of the simulation, using real timings for the SDRAM Module and SoC can indeed introduce the case where user accesses don't have time to be executed between refreshs. This situation will not happen on a real system and that was in fact the reason sys_clk_freq was ignored in the simulation. I'll review more carefully your patch and will think about the best solution (not sure we should introduce a refresh workaround on real systems, it's maybe better to only keep it for simulation).

enjoy-digital commented 3 years ago

@jfng: Sorry for the delay; I applied the first commit to use sys_clk_freq from the .yml file (https://github.com/enjoy-digital/litedram/commit/5aad6cd3d1ace3298ad3eb4d431a22f56578638e). The controller is not supposed to be operating below a minimal Clk/tREFI ratio (and would already have a terrible efficiency even before being close to this ratio), so I added an assertion to prevent generating the controller below a minimal ratio with https://github.com/enjoy-digital/litedram/commit/5d7adcfa7c88a4704f8a9225a5adc0cb2528ba59.

In simulation, even if your simulator is running at ~1MHz, you'll want to simulate with the same DRAM timings you'll be using on hardware. So while I understand the technical solution, it's not useful to introduce a workaround for the current behavior that is correct and just required the additional assert.

Thanks for the very clear report/PR.

enjoy-digital / litedram