pulp-platform / tech_cells_generic

Technology dependent cells instantiated in the design for generic process (simulation, FPGA)
Other
32 stars 30 forks source link

Reduce loop usage in sequential blocks #23

Closed meggiman closed 2 years ago

meggiman commented 2 years ago

This is an alternative to #20 which significantly reduces the loop count in the sequential block yet doesn't require the usage of blocking assignments. I verified this solution to work with the latest verilator version for the 2 port, 128 bit WordWidth and 16-bit BE width parametrization of tc_sram. If even wider memories are required the user should increase the --unroll-count parameter of Verilator as a work around.

niwis commented 2 years ago

Thank you a lot @meggiman! @JeanRochCoulon could you check whether this fixes your issue?

JeanRochCoulon commented 2 years ago

Hello @meggiman, By using your tc_sram model in a VCS simulation with CVA6 testharness testbench, the following error occurs:

Error-[ICPD] Illegal combination of drivers /home/jrcoulon/Projects/inner/tc_srams/core-v-cores/cva6/common/submodules/tc_sram.sv, 81 Illegal combination of procedural drivers Variable "sram" is driven by an invalid combination of procedural drivers. Variables written on left-hand of "always_ff" cannot be written to by any other processes, including other "always_ff" processes. This variable is declared at "/home/jrcoulon/Projects/inner/tc_srams/core-v-cores/cva6/common/submodules/tc_sram.sv", 81: logic[32'd63:0] sram[(NumWords - 1):0]; The first driver is at "/home/jrcoulon/Projects/inner/tc_srams/core-v-cores/cva6/corev_apu/tb/ariane_tb.sv", 150: ariane_tb.dut.i_sram.gen_cut[0].gen_mem.i_ram.sram[((address[23:0] >> 3) + i)] = mem_row; The second driver is at "/home/jrcoulon/Projects/inner/tc_srams/core-v-cores/cva6/common/submodules/tc_sram.sv", 125: always_ff @(posedge clk_i or negedge rst_ni) begin if (!rst_ni) begin ...

Today, I am not able to use this ram model because of this build error. This is due to the fact that the sram variable is driven by two verilog drivers. sram is loaded at the simulation start by https://github.com/openhwgroup/cva6/blob/master/corev_apu/tb/ariane_tb.sv through this verilator lines: define MAIN_MEM(P) dut.i_sram.gen_cut[0].gen_mem.i_ram.sram[(``P``)] ... MAIN_MEM((address[23:0] >> 3) + i) = mem_row (If I remove the above lines, the build is PASS)

By modifying the “write memory array” procedure by

In the case you are ok to introduce my modification, I could check whether everything works with my PR + your PR. Tell me!

Kind Regards

meggiman commented 2 years ago

Well, this is not a problem of the tc_sram but an issue of the cva6 testharness. The harness should use value deposit ($deposit) instead of assignment. The same tb would cause errors for any other kind of normal sequential element that use the best practice non-blocking assignment guidelines as well. Its not something tc_sram specific. We should fix problems at the root cause and not create workarounds in other modules. I guess an issue/pr in the cva6 repo is required. @niwis what do you think?

JeanRochCoulon commented 2 years ago

I have tried to replace MAIN_MEM((address[23:0] >> 3) + i) = mem_row; by $deposit(MAIN_MEM((address[23:0] >> 3) + i), mem_row);

I obtained the same error...

JeanRochCoulon commented 2 years ago

Have you already use the tc_sram model to preload code? In this case could you give me a URL link to see how it has been done.

meggiman commented 2 years ago

Strange. Deposit should not clash with non-blocking assignment. Do you have a branch where I can try it myself? The current master of cva6 doesn't use this model.

JeanRochCoulon commented 2 years ago

I have submitted a PR to replace CVA6 ram. Please see https://github.com/openhwgroup/cva6/pull/861 You can play with it ! Do not hesitate if needed

JeanRochCoulon commented 2 years ago

Merci Manuel et Nils !