Simulation hangs - Githubissues

davidgussler commented 8 months ago

Describe the bug The hello_world example program simulation hangs & times out after 10ms.

To Reproduce

Compile and install latest ghdl
Compile and install latest risc-v gcc toolchain
cd neorv32/sw/example/hello_world
make USER_FLAGS+=-DUART0_SIM_MODE MARCH=rv32imc_zicsr clean_all sim # Note: User guide section 13.4.1 does NOT specify that "_zicsr" extension needs to be used for the compilation, but I had to add it for the hello_world to compile. I'm unsure if this is an issue on my end or a problem with the documentation.

Console output: $ make USER_FLAGS+=-DUART0_SIM_MODE MARCH=rv32imc_zicsr clean_all sim ../../../sw/lib/source/neorv32_uart.c: In function 'neorv32_uart_setup': ../../../sw/lib/source/neorv32_uart.c:116:2: warning: #warning UART0_SIM_MODE (primary UART) enabled! Sending all UART0.TX data to text.io simulation output instead of real UART0 transmitter. Use this for simulations only! [-Wcpp] 116 | #warning UART0_SIM_MODE (primary UART) enabled! Sending all UART0.TX data to text.io simulation output instead of real UART0 transmitter. Use this for simulations only! | ^~~ Memory utilization: text data bss dec hex filename 3760 0 116 3876 f24 main.elf Compiling ../../../sw/image_gen/image_gen Installing application image to ../../../rtl/core/neorv32_application_image.vhd Simulating neorv32_application_image.vhd... Tip: Compile application with USER_FLAGS+=-DUART[0/1]_SIM_MODE to auto-enable UART[0/1]'s simulation mode (redirect UART output to simulator console). analyze ../../rtl/core/neorv32_package.vhd analyze ../../rtl/core/neorv32_application_image.vhd analyze uart_rx.simple.vhd analyze neorv32_tb.simple.vhd analyze ../../rtl/core/neorv32_clockgate.vhd analyze ../../rtl/core/neorv32_fifo.vhd analyze ../../rtl/core/neorv32_cpu_decompressor.vhd analyze ../../rtl/core/neorv32_cpu_control.vhd analyze ../../rtl/core/neorv32_cpu_regfile.vhd analyze ../../rtl/core/neorv32_cpu_cp_shifter.vhd analyze ../../rtl/core/neorv32_cpu_cp_muldiv.vhd analyze ../../rtl/core/neorv32_cpu_cp_bitmanip.vhd analyze ../../rtl/core/neorv32_cpu_cp_fpu.vhd analyze ../../rtl/core/neorv32_cpu_cp_cfu.vhd analyze ../../rtl/core/neorv32_cpu_cp_cond.vhd analyze ../../rtl/core/neorv32_cpu_alu.vhd analyze ../../rtl/core/neorv32_cpu_lsu.vhd analyze ../../rtl/core/neorv32_cpu_pmp.vhd analyze ../../rtl/core/neorv32_cpu.vhd analyze ../../rtl/core/neorv32_icache.vhd analyze ../../rtl/core/neorv32_dcache.vhd analyze ../../rtl/core/neorv32_intercon.vhd analyze ../../rtl/core/neorv32_dma.vhd analyze ../../rtl/core/neorv32_imem.entity.vhd analyze ../../rtl/core/neorv32_dmem.entity.vhd analyze ../../rtl/core/neorv32_boot_rom.vhd analyze ../../rtl/core/neorv32_xip.vhd analyze ../../rtl/core/neorv32_wishbone.vhd analyze ../../rtl/core/neorv32_cfs.vhd analyze ../../rtl/core/neorv32_sdi.vhd analyze ../../rtl/core/neorv32_gpio.vhd analyze ../../rtl/core/neorv32_wdt.vhd analyze ../../rtl/core/neorv32_mtime.vhd analyze ../../rtl/core/neorv32_uart.vhd analyze ../../rtl/core/neorv32_spi.vhd analyze ../../rtl/core/neorv32_twi.vhd analyze ../../rtl/core/neorv32_pwm.vhd analyze ../../rtl/core/neorv32_trng.vhd analyze ../../rtl/core/neorv32_neoled.vhd analyze ../../rtl/core/neorv32_xirq.vhd analyze ../../rtl/core/neorv32_gptmr.vhd analyze ../../rtl/core/neorv32_onewire.vhd analyze ../../rtl/core/neorv32_slink.vhd analyze ../../rtl/core/neorv32_crc.vhd analyze ../../rtl/core/neorv32_sysinfo.vhd analyze ../../rtl/core/neorv32_debug_dtm.vhd analyze ../../rtl/core/neorv32_debug_dm.vhd analyze ../../rtl/core/neorv32_top.vhd analyze ../../rtl/core/neorv32_bootloader_image.vhd analyze ../../rtl/core/mem/neorv32_imem.legacy.vhd analyze ../../rtl/core/mem/neorv32_dmem.legacy.vhd elaborate neorv32_tb_simple Using simulation run arguments: --stop-time=10ms ../../rtl/core/neorv32_clockgate.vhd:60:3:@0ms:(assertion warning): [NEORV32] Clock gating enabled (using generic clock switch). ../../rtl/core/neorv32_cpu_cp_bitmanip.vhd:172:3:@0ms:(assertion note): [NEORV32] Implementing bit-manipulation (B) sub-extensions Zba Zbb Zbc Zbs ../../rtl/core/neorv32_cpu_cp_fpu.vhd:292:3:@0ms:(assertion warning): [NEORV32] The floating-point unit (Zfinx) is still in experimental state. ../../rtl/core/neorv32_cpu.vhd:142:3:@0ms:(assertion note): [NEORV32] CPU ISA: rv32imabu_zicsr_zicntr_zicond_zifencei_zfinx_zihpm_zxcfu_sdext_sdtrig_smpmp ../../rtl/core/neorv32_cpu.vhd:163:3:@0ms:(assertion note): [NEORV32] CPU tuning options: fast_mul fast_shift ../../rtl/core/neorv32_cpu.vhd:170:3:@0ms:(assertion warning): [NEORV32] Assuming this is a simulation. ../../rtl/core/mem/neorv32_imem.legacy.vhd:72:3:@0ms:(assertion note): [NEORV32] Implementing LEGACY processor-internal IMEM as pre-initialized ROM. ../../rtl/core/neorv32_wishbone.vhd:117:3:@0ms:(assertion note): [NEORV32] Ext. Bus Interface (WISHBONE) - PIPELINED Wishbone protocol, auto-timeout, LITTLE-endian byte order, registered RX, registered TX ../../rtl/core/neorv32_trng.vhd:343:3:@0ms:(assertion note): [neoTRNG NOTE] << neoTRNG V3 - A Tiny and Platform-Independent True Random Number Generator >> ../../rtl/core/neorv32_trng.vhd:545:5:@0ms:(assertion warning): [neoTRNG WARNING] Implementing non-physical pseudo-RNG! ../../rtl/core/neorv32_trng.vhd:545:5:@0ms:(assertion warning): [neoTRNG WARNING] Implementing non-physical pseudo-RNG! ../../rtl/core/neorv32_trng.vhd:545:5:@0ms:(assertion warning): [neoTRNG WARNING] Implementing non-physical pseudo-RNG! ../../rtl/core/neorv32_debug_dm.vhd:227:3:@0ms:(assertion note): [NEORV32] OCD DM compatible to debug spec. version 1.0 ../../rtl/core/neorv32_top.vhd:355:5:@0ms:(assertion note): [NEORV32] The NEORV32 RISC-V Processor (version 0x01090503), github.com/stnolting/neorv32 ../../rtl/core/neorv32_top.vhd:361:5:@0ms:(assertion note): [NEORV32] Processor Configuration: IMEM DMEM I-CACHE D-CACHE WISHBONE GPIO MTIME UART0 UART1 SPI SDI TWI PWM WDT TRNG CFS NEOLED XIRQ GPTMR XIP ONEWIRE DMA SLINK CRC SYSINFO OCD ./neorv32_tb_simple:info: simulation stopped by --stop-time @10ms make: *** [../../../sw/common/common.mk:296: sim] Error 1

Expected behavior I would expect the terminal output from the hello_world example program from UG section 13.4.1

Screenshots NA

Environment: OS - Fedora 39 Workstation CPU intel i714700k

GHDL - $ ghdl -v GHDL 4.0.0-dev (3.0.0.r884.g2b9739682) [Dunoon edition] Compiled with GNAT Version: 13.2.1 20231205 (Red Hat 13.2.1- llvm 17.0.6 code generator Written by Tristan Gingold.

RISC-V GCC - $ riscv32-unknown-elf-gcc -v Using built-in specs. COLLECT_GCC=riscv32-unknown-elf-gcc COLLECT_LTO_WRAPPER=/opt/riscv/libexec/gcc/riscv32-unknown-elf/13.2.0/lto-wrapper Target: riscv32-unknown-elf Configured with: /home/david/prj/repos/riscv-gnu-toolchain/gcc/configure --target=riscv32-unknown-elf --prefix=/opt/riscv --disable-shared --disable-threads --disable-tls --enable-languages=c,c++ --with-system-zlib --with-newlib --with-sysroot=/opt/riscv/riscv32-unknown-elf --disable-libmudflap --disable-libssp --disable-libquadmath --disable-libgomp --disable-nls --disable-tm-clone-registry --src=.././gcc --disable-multilib --with-abi=ilp32 --with-arch=rv32i --with-tune=rocket --with-isa-spec=20191213 'CFLAGS_FOR_TARGET=-Os -mcmodel=medlow' 'CXXFLAGS_FOR_TARGET=-Os -mcmodel=medlow' Thread model: single Supported LTO compression algorithms: zlib gcc version 13.2.0 (GCC)

OS GCC - $ gcc -v Using built-in specs. COLLECT_GCC=gcc COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/13/lto-wrapper OFFLOAD_TARGET_NAMES=nvptx-none OFFLOAD_TARGET_DEFAULT=1 Target: x86_64-redhat-linux Configured with: ../configure --enable-bootstrap --enable-languages=c,c++,fortran,objc,obj-c++,ada,go,d,m2,lto --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-shared --enable-threads=posix --enable-checking=release --enable-multilib --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-gcc-major-version-only --enable-libstdcxx-backtrace --with-libstdcxx-zoneinfo=/usr/share/zoneinfo --with-linker-hash-style=gnu --enable-plugin --enable-initfini-array --with-isl=/builddir/build/BUILD/gcc-13.2.1-20231205/obj-x86_64-redhat-linux/isl-install --enable-offload-targets=nvptx-none --without-cuda-driver --enable-offload-defaulted --enable-gnu-indirect-function --enable-cet --with-tune=generic --with-arch_32=i686 --build=x86_64-redhat-linux --with-build-config=bootstrap-lto --enable-link-serialization=1 Thread model: posix Supported LTO compression algorithms: zlib zstd gcc version 13.2.1 20231205 (Red Hat 13.2.1-6) (GCC)

NEORV32 Git Hash (fresh checkout of main branch) - 8f3a2c3068ce087a6b46c998742d64f2388541d5

From the hello_world directory: make check ---------------- Check: NEORV32_HOME folder ---------------- NEORV32_HOME: ../../.. ---------------- Check: Shell ---------------- /bin/sh /usr/bin/bash ---------------- Check: riscv32-unknown-elf-gcc ---------------- Using built-in specs. COLLECT_GCC=riscv32-unknown-elf-gcc COLLECT_LTO_WRAPPER=/opt/riscv/libexec/gcc/riscv32-unknown-elf/13.2.0/lto-wrapper Target: riscv32-unknown-elf Configured with: /home/david/prj/repos/riscv-gnu-toolchain/gcc/configure --target=riscv32-unknown-elf --prefix=/opt/riscv --disable-shared --disable-threads --disable-tls --enable-languages=c,c++ --with-system-zlib --with-newlib --with-sysroot=/opt/riscv/riscv32-unknown-elf --disable-libmudflap --disable-libssp --disable-libquadmath --disable-libgomp --disable-nls --disable-tm-clone-registry --src=.././gcc --disable-multilib --with-abi=ilp32 --with-arch=rv32i --with-tune=rocket --with-isa-spec=20191213 'CFLAGS_FOR_TARGET=-Os -mcmodel=medlow' 'CXXFLAGS_FOR_TARGET=-Os -mcmodel=medlow' Thread model: single Supported LTO compression algorithms: zlib gcc version 13.2.0 (GCC) ---------------- Check: riscv32-unknown-elf-objdump ---------------- GNU objdump (GNU Binutils) 2.42 Copyright (C) 2024 Free Software Foundation, Inc. This program is free software; you may redistribute it under the terms of the GNU General Public License version 3 or (at your option) any later version. This program has absolutely no warranty. ---------------- Check: riscv32-unknown-elf-objcopy ---------------- GNU objcopy (GNU Binutils) 2.42 Copyright (C) 2024 Free Software Foundation, Inc. This program is free software; you may redistribute it under the terms of the GNU General Public License version 3 or (at your option) any later version. This program has absolutely no warranty. ---------------- Check: riscv32-unknown-elf-size ---------------- GNU size (GNU Binutils) 2.42 Copyright (C) 2024 Free Software Foundation, Inc. This program is free software; you may redistribute it under the terms of the GNU General Public License version 3 or (at your option) any later version. This program has absolutely no warranty. ---------------- Check: NEORV32 image_gen ---------------- NEORV32 executable image generator Three arguments are required. 1st: Operation -app_bin : Generate application executable binary (binary file, little-endian, with header) -app_img : Generate application raw executable memory image (vhdl package body file, no header) -raw_hex : Generate application raw executable (ASCII hex file, no header) -raw_bin : Generate application raw executable (binary file, no header) -bld_img : Generate bootloader raw executable memory image (vhdl package body file, no header) 2nd: Input file (raw binary image) 3rd: Output file 4th: Project name or folder (optional) ---------------- Check: Native GCC ---------------- Using built-in specs. COLLECT_GCC=gcc COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/13/lto-wrapper OFFLOAD_TARGET_NAMES=nvptx-none OFFLOAD_TARGET_DEFAULT=1 Target: x86_64-redhat-linux Configured with: ../configure --enable-bootstrap --enable-languages=c,c++,fortran,objc,obj-c++,ada,go,d,m2,lto --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-shared --enable-threads=posix --enable-checking=release --enable-multilib --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-gcc-major-version-only --enable-libstdcxx-backtrace --with-libstdcxx-zoneinfo=/usr/share/zoneinfo --with-linker-hash-style=gnu --enable-plugin --enable-initfini-array --with-isl=/builddir/build/BUILD/gcc-13.2.1-20231205/obj-x86_64-redhat-linux/isl-install --enable-offload-targets=nvptx-none --without-cuda-driver --enable-offload-defaulted --enable-gnu-indirect-function --enable-cet --with-tune=generic --with-arch_32=i686 --build=x86_64-redhat-linux --with-build-config=bootstrap-lto --enable-link-serialization=1 Thread model: posix Supported LTO compression algorithms: zlib zstd gcc version 13.2.1 20231205 (Red Hat 13.2.1-6) (GCC)

Toolchain check OK

Hardware: Default testbench HW

Additional context NA

Thank you! This is an unbelievable project that I've been quietly following for several years now. Now I finally have a use case for this processor, and I'm attempting (and failing) to get the basics up and running. Any help appreciated.

umarcor commented 8 months ago

See https://github.com/stnolting/neorv32/blob/main/.github/workflows/Processor.yml#L50-L74. Simulation is meant to be executed with software processor_check because the testbench does expect some specific UART output. Thus, it is not prepared to be simulated with arbitrary software.

BTW, it might be a good idea to enhance the documentation with some explanation and/or to improve the testbench(es) to support execution (simulation) of other software. I could reply so fast and easily just because @Unike267 did hit this "problem" some weeks ago and I had to check the CI/sources to understand it.

FTR, there is some content already in the UG:

davidgussler commented 8 months ago

Fantastic, thanks for the fast reply. Executing the "run_check.sh" script in the processor_check directory gave the results I'd expect.

So to make sure I'm clear on this, the only program that can be simulated at the HDL level is processor_check? All others can only be run on hardware (without additional improvements to the testbench / example programs)? If this is the case, then the UG sections that you linked are either out of date, or misleading because they make it seem like all of the example programs can be simulated with GHDL with their UART output sent to the terminal. I could definitely be misunderstanding. Apologies if I've missed something in the documentation.

umarcor commented 8 months ago

No worries, you are correct. We need to fix the code and/or update the docs. Since I have some spare time today, I'm running some simulations to try catch the problem. So far:

The make sim approach using the arguments described in the docs does fail for either processor_check or hello_world: https://github.com/umarcor/neorv32/actions/runs/7975576648
The CI job that executes the shell script uses doit to compile processor_check, and it does use some different arguments: https://github.com/stnolting/neorv32/blob/main/do.py#L29. Adding USER_FLAGS+=-DUART1_SIM_MODE and EFFORT=-Os and setting rv32ima_zicsr_zifencei instead of rv32imac_zicsr_zifencei makes a difference, since now processor_check passes; but hello_world still fails: https://github.com/umarcor/neorv32/actions/runs/7975739531. However, note that Hello world! :) is printed/shown (https://github.com/umarcor/neorv32/actions/runs/7975739531/job/21774642489#step:4:100), so I believe the problem is in the testbench. The simulation itself is usable if you check the log and ignore the exit code/result.

Still not clear what specific changes need to be done in the docs and/or in the code, but we are getting slightly closer.

FTR, the run_check.sh script that you mentioned uses arguments slightly different to the ones in the doit task: make USER_FLAGS+="-DUART0_SIM_MODE -DUART1_SIM_MODE -g -flto" EFFORT=-Os MARCH=rv32ima_zba_zbb_zbc_zbs_zicsr_zifencei_zicond clean_all all sim. The differences are -g and rv32ima_zba_zbb_zbc_zbs_zicsr_zifencei_zicond.

davidgussler commented 8 months ago

Got it, thanks so much for looking into this.

I copied the "run_check.sh" script into the hello_world dir, ran it, and saw the hello world program run correctly. So it appears to have been a problem with the arguments as you alluded to (but it is still returning with exit failure status for me too).

Also BTW, I've seen you all over several other open source FPGA repos, so thanks for all of your contributions to the ecosystem in general.

stnolting commented 8 months ago

Hey @davidgussler!

I think this is just a configuration issue. You have compiled your application (hello_world) with the C ISA extension enabled:

Console output: $ make USER_FLAGS+=-DUART0_SIM_MODE MARCH=rv32imc_zicsr clean_all sim

But the default testbench is configured without the C ISA extension. Your GHDL log also shows the actual hardware configuration:

../../rtl/core/neorv32_cpu.vhd:142:3:@0ms:(assertion note): [NEORV32] CPU ISA: rv32imabu_zicsr_zicntr_zicond_zifencei_zfinx_zihpm_zxcfu_sdext_sdtrig_smpmp

The compiler has generated compressed instructions but the CPU hardware is not capable of executing them so the whole processor hangs in an eternal exception loop.

Just re-compile and re-run your application with the right ISA flags: make USER_FLAGS+=-DUART0_SIM_MODE MARCH=rv32im_zicsr clean_all sim

Note: User guide section 13.4.1 does NOT specify that "_zicsr" extension needs to be used for the compilation, but I had to add it for the hello_world to compile. I'm unsure if this is an issue on my end or a problem with the documentation.

Right, the documentation is not very precise here. _zicsr needs to be in the MARCH string all the time.

So to make sure I'm clear on this, the only program that can be simulated at the HDL level is processor_check?

You can run any program you like using the makefile's sim target. You just might need to adjust the testbench configuration (for example enabling the C extension):

https://github.com/stnolting/neorv32/blob/f4a6365e3827f52dab890f14f422fc54836e1bbb/sim/simple/neorv32_tb.simple.vhd#L174

Thank you! This is an unbelievable project that I've been quietly following for several years now.

Thank you very much! 😃

stnolting commented 8 months ago

FTR

The GHDL script that are called by "make sim" were originally designed for running the processor_check program only. At the very end these scripts check for a specific UART string to tell if the simulation was successful. Obviously, this check does not work when simulating any other program (like hello_word) so the Makefile (or more precisely, the GHDL scripts) always return an error after timeout. This is misleading (but not harmful), so we should really fix this 🙈

umarcor commented 8 months ago

@stnolting I suggest we check for some envvar (say CI=true, see https://github.com/umarcor/neorv32/actions/runs/7976961344/job/21778719141#step:4:267) in order to call the cat-grep "assert". Or, alternatively, we move the cat-grep assert to the processor_check makefile, instead of having it in the script which is called by any "program".

On the other hand, since the default testbench has extension C disabled, we should update the docs and use MARCH=rv32im or MARCH=rv32ima in https://stnolting.github.io/neorv32/ug/#_hello_world. We might need to check it somewhere else.

We should probably also add a warning/note about the whole processor hanging in an eternal exception loop if unsupported/unconfigured instructions are used.

Last, shall we add some of the example software executions to CI (e.g. hello_world and demo_blink_led) in order to catch these issues in the future?

@davidgussler thanks for your kind words!

stnolting commented 8 months ago

I suggest we check for some envvar [...]

I was about to pass the folder of the software executable as another argument to the GHDL script and execute the cat-grep only if that is equal to "processor_check"... But your proposal seems to be much cleaner! :+1:

On the other hand, since the default testbench has extension C disabled, we should update the docs and use MARCH=rv32im or MARCH=rv32ima in https://stnolting.github.io/neorv32/ug/#_hello_world. We might need to check it somewhere else.

Good point. I'll update that.

We should probably also add a warning/note about the whole processor hanging in an eternal exception loop if unsupported/unconfigured instructions are used.

Hm, also a good point. But where to put that? Any suggestions?

Last, shall we add some of the example software executions to CI (e.g. hello_world and demo_blink_led) in order to catch these issues in the future?

The demo_blink_led does not output anything via UART - so there is nothing to see there.

If we add "hello_world" to the CI checks then we would also need another "checker" to validate there is some (expected) UART output. Where should we put this?

umarcor commented 8 months ago

@stnolting I believe I addressed all the points in #817 and #819, except running hello_world in CI.

davidgussler commented 8 months ago

Great info. Another enhancement could be to add a readme for each example program that specifies the minimum VHDL generic configuration that's required to run that specific application.

umarcor commented 8 months ago

@davidgussler I like that. Yet, instead of a README per subdir, I would suggest a "table" in a single sw/example/march.txt. For instance:

hello_world=rv32imac
processor_check=rv32ima

The point is for the file to be machine-readable, so that we can eventually use it to automate either tests or checks.

davidgussler commented 8 months ago

@umarcor Much better! I like it. But along with the architecture specification, I think that you would also want to have a way of capturing the optional bus peripherals and any other generic options that could affect the outcome of the simulation.

The easiest way to do this (that is still machine readable) might be have an individual VUnit regression for each test app (where feasible... I know for example, that @stnolting mentioned that some of the apps don't give any uart output), where the python test runner passes the required generics to the TB depending on test that needs to be run. This way, you don't need a table or readme because the simulation configurations are "self-documented" in a way.

Just spit-balling here. I'm not familiar with the inner-workings of this project, so maybe this doesn't fit with project goals / would be too much of a pain to set up. This may not be worth the effort if the standard riscv isa processor test is already enough verification.

stnolting commented 8 months ago

Great info. Another enhancement could be to add a readme for each example program that specifies the minimum VHDL generic configuration that's required to run that specific application.

That's a good idea, but I think it is not really necessary. At least when it comes to the CPU ISA extensions you can always use the default minimal ISA configuration:

https://github.com/stnolting/neorv32/blob/f4a6365e3827f52dab890f14f422fc54836e1bbb/sw/common/common.mk#L52-L53

To actually see something when doing an "in-console" simulation using GHDL (using the make sim target) you need the UART's simulation mode (USER_FLAGS+=-DUART0_SIM_MODE) - but beware, not all example programs produce UART outputs.

I'm not really sure if we really need to run all example programs by the CI pipeline. I mean, that would be nice to ensure everything still works after a PR. But maybe this is even impossible as some example programs need explicit user inputs... E.g. how can you check the "demo_twi" program without having a user input that triggers the actual TWI transfers and without having a simulation model of a TWI chip?

umarcor commented 8 months ago

I'm not really sure if we really need to run all example programs by the CI pipeline. I mean, that would be nice to ensure everything still works after a PR. But maybe this is even impossible as some example programs need explicit user inputs... E.g. how can you check the "demo_twi" program without having a user input that triggers the actual TWI transfers and without having a simulation model of a TWI chip?

I would add just the ones that make sense. If that's just hello_world, it's ok. In the end, that's the one mentioned in the docs.

With regard to TWI or similar use cases, that should be done with a verification component, so that would be an specific testbench managed by VUnit, instead of the simple testbench.

umarcor commented 8 months ago

@davidgussler I'm sorry I had missed https://github.com/stnolting/neorv32/issues/816#issuecomment-1954801302.

I agree. That's the reason for having VUnit in this repo: to handle multiple testbenches from module/unit tests to integration tests. Currently, there are not many testbenches testing each of the peripherals, but I believe it would be welcome to eventually walk there. Some will be rather straightforward (such as the hello_world), others will be more complex (because there might not be Verification Components available for it).

Moreover, we currently build the software and then run the simulation, in separated steps. I believe that's because the sim container did not contain the RISCV GCC at first. That was a separated environment, and therefore a separated step. However, we could make run.py call make and we could have it customised for an specific test or set of tests through the pre hook (http://vunit.github.io/py/ui.html#pre-and-post-simulation-hooks).

If you want to take the task, I believe a good starting point would be a testbench that exposes the cpu extension generics, as you mentioned. Actually, instead of exposing all of them through generics, you might want to consider using JSON-for-VHDL. There is an example at http://vunit.github.io/examples.html#json-for-vhdl which shows how to pass multiple arguments/parameters as either a JSON file or an encoded JSON string. Note that JSON-for-VHDL is synthesisable, so that mechanism might be also used for a single parameterised top-level that is configurable through a JSON file.

davidgussler commented 8 months ago

@umarcor Got it. No promises on getting to this any time soon, but I'll keep it on my radar. Especially considering @snolting's last comment where he pointed out that essentially all of the apps should run with the default configuration, it seems like exposing the DUT generics to the python runner may just end up adding more complexity without contributing a worthwhile benefit.

Thanks again for all of the help today. I'll go ahead and close this issue now that the documentation has been updated. I think that a separate issue / enhancement could be created to add the hello_world to the CI suite if that is still something you see as worthwhile.

stnolting / neorv32

Simulation hangs #816