tillitis / tillitis-key1

Board designs, FPGA verilog, firmware for TKey, the flexible and open USB security key 🔑
https://www.tillitis.se
382 stars 24 forks source link

FPGA: Add sim model of udi_rom #227

Closed secworks closed 4 weeks ago

secworks commented 1 month ago

This PR adds a simulation model of the udi_rom. This allows us to build the testbench and simulated the tk1 core. This PR does not change the actual FPGA design.

dehanj commented 1 month ago

When I run this testbench I get

--- test2: Read out UDI started.
--- Error: Got 0x1337f00f when reading from 0x030, expected 0x00010203

--- Error: Got 0xdeadbeef when reading from 0x031, expected 0x04050607

--- test2: completed.
secworks commented 1 month ago

Doh! I just checked that it fixed the build issue.

secworks commented 1 month ago

The UDI ROM testcase now works.

dehanj commented 1 month ago

Tried again, test case 2 seems to work. Some things I noted:

  1. Running the linter in the same folder yields these results:
    make lint-top
    verilator +1364-2005ext+ --lint-only  -Wall -Wno-fatal -Wno-DECLFILENAME ../rtl/tk1.v
    %Error: ../rtl/tk1.v:181:3: Cannot find file containing module: 'SB_RGBA_DRV'
    181 |   SB_RGBA_DRV #(
      |   ^~~~~~~~~~~
    %Error: ../rtl/tk1.v:181:3: This may be because there's no search path specified with -I<dir>.
    181 |   SB_RGBA_DRV #(
      |   ^~~~~~~~~~~
        ... Looked in:
             SB_RGBA_DRV
             SB_RGBA_DRV.v
             SB_RGBA_DRV.sv
             obj_dir/SB_RGBA_DRV
             obj_dir/SB_RGBA_DRV.v
             obj_dir/SB_RGBA_DRV.sv
    %Error: ../rtl/tk1.v:199:5: Cannot find file containing module: 'udi_rom'
    199 |     udi_rom rom_i(
      |     ^~~~~~~
    %Error: Exiting due to 3 error(s)
    make: *** [Makefile:36: lint-top] Error 1

Answer from Joachim: The issue with these two is hat we want to lint the real code that is used to build the FPGA, not simulation models. For both the udi_rom and the SB_RBGA, they basically instantiates FPGA primitive that are linked by Yosys in its cell mapping stage. So there exists no code that can be linted. We could add IGNORE-pramgas to make the linter to progress. och provide a model for the linter. And I see that as something not related to the simulation model this PR handles.

secworks commented 1 month ago

Tried again, test case 2 seems to work. Some things I noted:

1. I get these warnings while running `make` in `hw/application_fpga/core/tk1/toolruns`

../rtl/tk1.v:352: warning: Extra digits given for sized hex constant. ../rtl/tk1.v:504: warning: @* is sensitive to all 8 words in array 'cdi_mem'.

Is that something that should be fixed?

The first one: Yes. But not in this PR. It is not related to the UDI simulation model.

The second one: No. It is expected behaviour. Its a combinational block, which means that any change should update the simulation status. This makes the simulator have to work more. So the warning is to inform the designer that this has been detected, so any unintentional behaviour is not missed.