Parameter and pinout cleanup #277 #278

Silabs-ArjanB · 2020-03-27T11:49:06Z

Signed-off-by: Arjan Bink [email protected]

Signed-off-by: Arjan Bink <[email protected]>

Silabs-ArjanB · 2020-03-27T11:49:46Z

Needs to be done at the same time as openhwgroup/core-v-verif#85

Signed-off-by: Arjan Bink <[email protected]>

Silabs-ArjanB · 2020-03-30T14:55:13Z

Added back in the APU interface.

davideschiavone · 2020-03-31T12:30:33Z

Thanks a lot @Silabs-ArjanB ,

few comments here:

The tb/dm testbench hasn't been updated with the new interface and parameters
Fixing PULP_SECURE to 0 makes the MTVEC(and MTVECx) tight to Boot Address, this makes the tests failing. It's a good example for issue mtvec is tied to boot_addr_i when PULP_SECURE=0 #254
We need to fix mtvec is tied to boot_addr_i when PULP_SECURE=0 #254 before having this PR merged. So I opened in parallel a PR for mtvec is tied to boot_addr_i when PULP_SECURE=0 #254 , then you can merge it into your branch to have this point solved.

Silabs-ArjanB · 2020-03-31T12:50:46Z

The status of the tb/dm is unclear to me. They already seemed out of date to begin with. Can you let me know which ones are actually in use and I will update them (some of them seem to have been copied into the core-v-verif repos instead).

Do you want me to work on #254?

davideschiavone · 2020-03-31T13:02:10Z

PR for #254 is already here: #285
You just need to review.

tb/dm should just compile and work as before. The purpose is to provide an example to use GDB.
The tb folder will be replaced with the example name as its content is not for verification but just to provide quick examples. One with GDB/Debug and one with hello world.

davideschiavone · 2020-04-02T12:27:21Z

@Silabs-ArjanB , as the PR #285 has been merged to master, the point 2. should be solved.

2. Fixing PULP_SECURE to 0 makes the MTVEC(and MTVECx) tight to Boot Address, this makes the tests failing. It's a good example for issue #254
   We need to fix #254 before having this PR merged. So I opened in parallel a PR for #254 , then you can merge it into your branch to have this point solved.

You should now merge the master to your branch and see if it solves the issue and address the point 1 here:

1. The tb/dm testbench hasn't been updated with the new interface and parameters

Thanks @Silabs-ArjanB

…cleanup

Signed-off-by: Arjan Bink <[email protected]>

Silabs-ArjanB · 2020-04-02T15:22:03Z

Cleaned up parameters and pinout in various testbenches.

make vsim-run in 'cv32e40p/tb/dm' now compiles without errors and warnings. C code comilation however fails (I expect it was already like this)
make custom-vsim-run in cv32e40p/tb/core now compiles without errors and warnings and executes okay as well.
I don't have verilator so I couldn't check those compile targets (but updated all testbenches)
make firmware-vsim-run-gui in cv32e40p/tb/core has gcc compilation issues (maybe a Makefile issue as 64-bit code is getting compiled it seems).

Could we open another ticket to get rid of the high amount of testbenches and duplication in cv32e40p. For example riscv_core is currently being instantiated in all of the following:

cv32e40p/tb/core/riscv_wrapper.sv
cv32e40p/tb/dm/tb_test_env.sv
cv32e40p/tb/dm/tb_top_verilator.sv
cv32e40p/tb/verilator-model/top.sv
cv32e40p/tb/tb_riscv/tb_riscv_core.sv

and riscv_wrapper for example is then being instantiated in

tb/core/tb_top.sv
tb/core/tb_top_verilator.sv

I updated all of the above but we should try to get rid of obsolete testbenches.

davideschiavone · 2020-04-03T09:43:32Z

Hi @Silabs-ArjanB , my answers inline

Cleaned up parameters and pinout in various testbenches.

* make vsim-run in 'cv32e40p/tb/dm' now compiles without errors and warnings. C code comilation however fails (I expect it was already like this

It works for me

* make custom-vsim-run in cv32e40p/tb/core now compiles without errors and warnings and executes okay as well.

👍

* I don't have verilator so I couldn't check those compile targets (but updated all testbenches)

* make firmware-vsim-run-gui in cv32e40p/tb/core has gcc compilation issues (maybe a Makefile issue as 64-bit code is getting compiled it seems).

The problem is the RVA instructions emitted. As you tight to 0 the RVA parameter, you should also disable those instructions.
To do so, please remove them from firmware/start.S and from the Makefile variables
RISCV_TEST_INCLUDES and FIRMWARE_TEST_OBJS

Could we open another ticket to get rid of the high amount of testbenches and duplication in cv32e40p. For example riscv_core is currently being instantiated in all of the following:

cv32e40p/tb/core/riscv_wrapper.sv
cv32e40p/tb/dm/tb_test_env.sv
cv32e40p/tb/dm/tb_top_verilator.sv
cv32e40p/tb/verilator-model/top.sv
cv32e40p/tb/tb_riscv/tb_riscv_core.sv

and riscv_wrapper for example is then being instantiated in

tb/core/tb_top.sv
tb/core/tb_top_verilator.sv

I updated all of the above but we should try to get rid of obsolete testbenches.

Yes please go ahead. We already have a ticket to change name to the folder and clean it up. @MikeOpenHWGroup is porting the interrupt test. Then, we should only have an example of application and the debug connected.

Silabs-ArjanB · 2020-04-03T10:07:33Z

Hi @davideschiavone , thank you for your feedback.

I will fix the RVA instruction issue as you described.

Can you be more specific about which parts I can remove please? For example, can I remove all of the following?

Files:

cv32e40p/tb/dm/tb_top_verilator.sv
tb/core/tb_top_verilator.sv

Directories:

cv32e40p/tb/verilator-model
cv32e40p/tb/serDiv
cv32e40p/tb/tb_MPU
cv32e40p/tb/tb_riscv

davideschiavone · 2020-04-03T10:39:40Z

Hi @Silabs-ArjanB ,

Can we have that discussion in another thread/issue ?

MikeOpenHWGroup · 2020-04-03T12:55:19Z

rtl/riscv_core.sv

+  output logic [31:0] instr_addr_o,
+  input  logic [31:0] instr_rdata_i,


Why are we removing parameters here? Even if these widths must be fixed, there are a lot of magic numbers in the code and it would be a good idea to slowly replace them all. How about:

output logic instr_req_o, input logic instr_gnt_i, input logic instr_rvalid_i, output logic [INSTR_ADDR_WIDTH -1:0] instr_addr_o, input logic [INSTR_RDATA_WIDTH-1:0] instr_rdata_i,

Only instr_rdata_i had a parameter. There is no need to use a parameter for instr_addr_o. This will always be 32 bits wide. The use of INSTR_ADDR_WIDTH is debatable; I would not be against it but at the same time both cv32e40p and cv32e40 will have it set to 32.

MikeOpenHWGroup · 2020-04-03T12:56:47Z

rtl/riscv_core.sv

@@ -91,8 +74,6 @@ module riscv_core
  output logic [31:0] data_wdata_o,


More magic numbers. Consider replacing with parameter, even if these are intended to be fixed values for cv32e.

It is a 32-bit core. Let's not go adding even more parameters

MikeOpenHWGroup · 2020-04-03T12:58:01Z

rtl/riscv_core.sv

@@ -476,7 +476,7 @@ module riscv_core
    .N_HWLP              ( N_HWLP            ),
    .RDATA_WIDTH         ( INSTR_RDATA_WIDTH ),
    .FPU                 ( FPU               ),
-    .DM_HaltAddress      ( DM_HaltAddress    )
+    .DM_HALTADDRESS      ( DM_HALTADDRESS    )


Nice. All caps for parameters is a good coding style.

MikeOpenHWGroup · 2020-04-03T12:58:10Z

rtl/riscv_core.sv

@@ -561,7 +561,7 @@ module riscv_core
    .A_EXTENSION                  ( A_EXTENSION          ),
    .APU                          ( APU                  ),
    .FPU                          ( FPU                  ),
-    .Zfinx                        ( Zfinx                ),
+    .PULP_ZFINX                   ( PULP_ZFINX           ),


Silabs-ArjanB · 2020-04-03T13:30:09Z

Hi @Silabs-ArjanB ,

Can we have that discussion in another thread/issue ?

Sure

…uration) Signed-off-by: Arjan Bink <[email protected]>

Silabs-ArjanB · 2020-04-03T13:40:49Z

Fixed Makefile and firmware/start.S to no longer use non-supported instructions (i.e. atomics). Note that riscv_tests/rv32uc/rvc.S needed to be changed (as was done before) as it led to an internal compiler error.

davideschiavone · 2020-04-03T13:54:56Z

For me it's ok to MERGE, just hitting @bluewww to ask him about the last change you have done in the RVC (he is the expert :) )

bluewww · 2020-04-07T09:15:51Z

tb/core/firmware/start.S

-	TEST(amoswap_w)
-	TEST(amoxor_w)
-	TEST(lrsc)
-


Why is this removed? These test should work. Also I don't think a PR that is called cleanup should regress on features.

The RVA parameter has been set to 0 by default, so they generate illegal instructions now

bluewww · 2020-04-07T20:29:29Z

tb/core/mm_ram.sv

@@ -715,7 +716,7 @@ module mm_ram
    .req_core_i         ( instr_req_i            ),
    .req_mem_o          ( rnd_stall_instr_req    ),

-    .addr_core_i        ( 32'(instr_addr_i)      ),


Removing this will generate a port mismatch warning

The connections to riscv_random_stall were actually generating port mismatch warnings and they have been fixed by above change. The key is that I also added an ADDR_WIDTH parameter to riscv_random_stall so that it can be used in different context with potentially different address with requirements (so that we can make this warning free in every context).

bluewww · 2020-04-07T20:39:56Z

tb/core/riscv_tests/rv32uc/rvc.S


-#include "../rv64uc/rvc.S"


Is this a copy paste of the ../rv64uc/rvc.S file? Why would you do this for this file, but then you don't do it for the others?

There is a very good reason the structure of the test files is like this (i.e. with the #includes from rv64uc). They are a verbatim import from https://github.com/riscv/riscv-tests meaning that whenever there is an update to the upstream test repository, you can easily import the diff. Now that you change the structure you have to apply fixes everytime you do this. This is just an unnecessary increase in maintenance cost.

All in all its inconsistent and a bad idea. I would highly suggest to revert this change and apply the fix to ../rv64uc/rvc.S which I guess is removing the .option norelax, a fix I mistakenly did because I compiled with -march=rv32im instead of imc I suspect.

I fully agree that it is important to be able to enable a verbatim import from https://github.com/riscv/riscv-tests. That being said, the tests (together with our Makefile/compiler) were not compiling correctly.

The ./rv64uc/rvc.S and ./rv32uc/rvc.S file are not the same. Even on the master branch this test case led to a gcc internal error, so the starting point was a repos in which test cases were not compiling correctly.

I looked at what this test looked like roughly half a year ago and I re-applied the same change to get it to work. (This might be related to this issue: riscv-collab/riscv-gnu-toolchain#445)

diff ./rv64uc/rvc.S ./rv32uc/rvc.S
10a11,12

#undef RVTEST_RV64U
#define RVTEST_RV64U RVTEST_RV32U
16,18d17
< // riscv-collab/riscv-gnu-toolchain#445
< // mixed modes should turn of relaxation (?)
< .option norelax

Originally I also thought it had to do with the missing imc switch so I fixed that first. However, if we keep the test as it was the entire test suite will not run because I would still get the following error:

/tool/pulp_riscv_gnu_toolchain/2019.12/rhel6-64/bin/riscv32-unknown-elf-gcc -c -march=rv32imc -g -o riscv_tests/rv32uc/rvc.o
-Iriscv_tests/ -Iriscv_tests/macros/scalar
-DTEST_FUNC_NAME=rvc
-DTEST_FUNC_TXT='"rvc"'
-DTEST_FUNC_RET=rvc_ret riscv_tests/rv32uc/rvc.S
riscv32-unknown-elf-gcc: internal compiler error: Floating point exception (program as)
Please submit a full bug report,
with preprocessed source if appropriate.
See https://gcc.gnu.org/bugs/ for instructions.
make: *** [riscv_tests/rv32uc/rvc.o] Error 4

I will file a separate issue on the compilation issue with the rvc.S test (and I acknowledge that I should already have done that). In the mean time I think it is important to have the repos in a state that actually runs with 0 errors and 0 warnings.

This might also be a pulp-riscv-gnu-toolchain bug. I believe that the test worked with riscv-gnu-toolchain but I would have to check again.

bluewww · 2020-04-07T20:40:38Z

tb/core/riscv_wrapper.sv

@@ -42,7 +43,7 @@ module riscv_wrapper
    logic [3:0]                   data_be;
    logic [31:0]                  data_rdata;
    logic [31:0]                  data_wdata;
-    logic [5:0]                   data_atop;
+    logic [5:0]                   data_atop = 'b0;


indicate bitwidth. Should stay consistent in this repo.

Okay, will do

bluewww · 2020-04-07T20:40:58Z

tb/core/Makefile

@@ -371,7 +369,7 @@ riscv_tests/rv32um/%.o: riscv_tests/rv32um/%.S riscv_tests/riscv_test.h \

 riscv_tests/rv32uc/%.o: riscv_tests/rv32uc/%.S riscv_tests/riscv_test.h \
 			riscv_tests/macros/scalar/test_macros.h
-	$(RISCV_EXE_PREFIX)gcc -c -march=rv32im -g -o $@ \
+	$(RISCV_EXE_PREFIX)gcc -c -march=rv32imc -g -o $@ \


bluewww · 2020-04-07T20:41:15Z

tb/core/Makefile

@@ -83,15 +83,13 @@ RISCV_EXE_PREFIX         = $(RISCV)/bin/riscv32-unknown-elf-

 # firmware vars
 FIRMWARE                 = firmware/
-RISCV_TEST_INCLUDES      = -Iriscv_tests/ -Iriscv_tests/macros/scalar \
-				-Iriscv_tests/rv64ui -Iriscv_tests/rv64um -Iriscv_tests/rv64ua


Revert this too, see comment above

Agreed this should be reverted (which has been done now). The rv64* includes are however not actually being used yet as I am still not using the originally intended rvc.S (see discussion above). Once we get the rvc.S issue solved, then at least RISCV_TEST_INCLUDES will be already have the correct value.

bluewww · 2020-04-07T20:42:04Z

tb/dm/Makefile

@@ -30,7 +30,7 @@ VLOG_FLAGS		= -pedanticerrors -suppress 2577 -suppress 2583
 VLOG_LOG                = vloggy

 VOPT			= vopt-$(VVERSION)
-VOPT_FLAGS		= -debugdb -fsmdebug -pedanticerrors #=mnprft
+VOPT_FLAGS		= -debugdb -fsmdebug +acc -pedanticerrors #=mnprft


Ok this makes sense otherwise the defaults would be really useless.

bluewww · 2020-04-07T20:42:21Z

tb/dm/tb_test_env.sv

-        .apu_master_flags_i     (                       ),
-
+        .apu_master_valid_i     ( 1'b0                  ),
+        .apu_master_result_i    (  'b0                  ),


indicate bitwidth. Should stay consistent in this repo.

Fixed (and made similar fixes in other places)

bluewww · 2020-04-07T20:42:36Z

tb/dm/tb_top_verilator.sv

-        .apu_master_flags_i     (                       ),
-
+        .apu_master_valid_i     ( 1'b0                  ),
+        .apu_master_result_i    (  'b0                  ),


indicate bitwidth

tb/tb_riscv/riscv_random_stall.sv

bluewww · 2020-04-07T20:46:42Z

tb/verilator-model/top.sv

-      .apu_master_flags_i     (                       ),
-
+      .apu_master_valid_i     ( 1'b0                  ),
+      .apu_master_result_i    (  'b0                  ),


indicate bitwidth

Signed-off-by: Arjan Bink <[email protected]>

davideschiavone · 2020-04-08T17:39:05Z

Actually, these changes in the RISCV_TESTS suite should be done in the core-verif repository, the only purpose of this "testbench" should be make hello world pass. So it's not wasted work as can be ported to core-verif, but probably we can ignore the them here

Parameter and pinout cleanup openhwgroup#277

c81bb7f

Signed-off-by: Arjan Bink <[email protected]>

This was referenced Mar 27, 2020

Parameter and pinout cleanup #277 openhwgroup/core-v-verif#85

Closed

Clean up parameters and pinout #277

Closed

Parameter and pinout cleanup (re-added APU interface) openhwgroup#277

9e8fb61

Signed-off-by: Arjan Bink <[email protected]>

MikeOpenHWGroup mentioned this pull request Apr 2, 2020

Parameter and pinout cleanup: take2 openhwgroup/core-v-verif#90

Merged

Silabs-ArjanB added 2 commits April 2, 2020 08:21

Merge remote-tracking branch 'upstream/master' into ArjanB_parameter_…

df1ce89

…cleanup

Parameter and pinout cleanup in various testbenches

2fc2477

Signed-off-by: Arjan Bink <[email protected]>

MikeOpenHWGroup reviewed Apr 3, 2020

View reviewed changes

Removed unused test cases (no longer supported in the cv32e40p config…

615a032

…uration) Signed-off-by: Arjan Bink <[email protected]>

bluewww reviewed Apr 7, 2020

View reviewed changes

bluewww suggested changes Apr 7, 2020

View reviewed changes

Made bit widths explicit. Partially reverted Makefile changes

117d01f

Signed-off-by: Arjan Bink <[email protected]>

Silabs-ArjanB requested a review from bluewww April 8, 2020 06:53

davideschiavone merged commit 8666456 into openhwgroup:master Apr 10, 2020

Silabs-ArjanB deleted the ArjanB_parameter_cleanup branch April 11, 2020 06:41

Silabs-ArjanB restored the ArjanB_parameter_cleanup branch April 11, 2020 07:22

Silabs-ArjanB deleted the ArjanB_parameter_cleanup branch April 14, 2020 14:33

		output logic [31:0] instr_addr_o,
		input logic [31:0] instr_rdata_i,

		@@ -91,8 +74,6 @@ module riscv_core
		output logic [31:0] data_wdata_o,


		#include "../rv64uc/rvc.S"

Parameter and pinout cleanup #277 #278

Parameter and pinout cleanup #277 #278

Conversation

Silabs-ArjanB commented Mar 27, 2020

Silabs-ArjanB commented Mar 27, 2020

Silabs-ArjanB commented Mar 30, 2020

davideschiavone commented Mar 31, 2020

Silabs-ArjanB commented Mar 31, 2020

davideschiavone commented Mar 31, 2020

davideschiavone commented Apr 2, 2020

Silabs-ArjanB commented Apr 2, 2020

davideschiavone commented Apr 3, 2020 • edited Loading

Silabs-ArjanB commented Apr 3, 2020

davideschiavone commented Apr 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Silabs-ArjanB commented Apr 3, 2020

Silabs-ArjanB commented Apr 3, 2020

davideschiavone commented Apr 3, 2020

bluewww Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Silabs-ArjanB Apr 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluewww Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluewww Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davideschiavone commented Apr 8, 2020

davideschiavone commented Apr 3, 2020 •

edited

Loading

bluewww Apr 7, 2020 •

edited

Loading

Silabs-ArjanB Apr 8, 2020 •

edited

Loading

bluewww Apr 7, 2020 •

edited

Loading

bluewww Apr 7, 2020 •

edited

Loading