Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parameter and pinout cleanup #277 #278

Merged
merged 6 commits into from
Apr 10, 2020
Merged

Parameter and pinout cleanup #277 #278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c81bb7f
Parameter and pinout cleanup #277
Silabs-ArjanB Mar 27, 2020
9e8fb61
Parameter and pinout cleanup (re-added APU interface) #277
Silabs-ArjanB Mar 30, 2020
df1ce89
Merge remote-tracking branch 'upstream/master' into ArjanB_parameter_…
Silabs-ArjanB Apr 2, 2020
2fc2477
Parameter and pinout cleanup in various testbenches
Silabs-ArjanB Apr 2, 2020
615a032
Removed unused test cases (no longer supported in the cv32e40p config…
Silabs-ArjanB Apr 3, 2020
117d01f
Made bit widths explicit. Partially reverted Makefile changes
Silabs-ArjanB Apr 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions rtl/include/apu_core_package.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,13 @@ package apu_core_package;
// until here //
/////////////////////////////////////////////////////////////////////////////

// APU interface
parameter WAPUTYPE = 0;
parameter APU_NARGS_CPU = 3;
parameter APU_WOP_CPU = 6;
parameter APU_NDSFLAGS_CPU = 15;
parameter APU_NUSFLAGS_CPU = 5;

// FP-general
parameter APU_FLAGS_FP = 2;
parameter APU_FLAGS_FPNEW = 3;
Expand Down
4 changes: 2 additions & 2 deletions rtl/register_file_test_wrap.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ module register_file_test_wrap
parameter ADDR_WIDTH = 5,
parameter DATA_WIDTH = 32,
parameter FPU = 0,
parameter Zfinx = 0
parameter PULP_ZFINX = 0
)
(
// Clock and Reset
Expand Down Expand Up @@ -134,7 +134,7 @@ module register_file_test_wrap
.ADDR_WIDTH ( ADDR_WIDTH ),
.DATA_WIDTH ( DATA_WIDTH ),
.FPU ( FPU ),
.Zfinx ( Zfinx )
.PULP_ZFINX ( PULP_ZFINX )
)
riscv_register_file_i
(
Expand Down
70 changes: 35 additions & 35 deletions rtl/riscv_core.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,27 +37,10 @@ import riscv_defines::*;

module riscv_core
#(
parameter N_EXT_PERF_COUNTERS = 0,
parameter INSTR_RDATA_WIDTH = 32,
parameter PULP_SECURE = 0,
parameter N_PMP_ENTRIES = 16,
parameter USE_PMP = 1, //if PULP_SECURE is 1, you can still not use the PMP
parameter PULP_CLUSTER = 1,
parameter A_EXTENSION = 0,
parameter PULP_CLUSTER = 0,
parameter FPU = 0,
parameter Zfinx = 0,
parameter FP_DIVSQRT = 1,
parameter SHARED_FP = 0,
parameter SHARED_DSP_MULT = 0,
parameter SHARED_INT_MULT = 0,
parameter SHARED_INT_DIV = 0,
parameter SHARED_FP_DIVSQRT = 0,
parameter WAPUTYPE = 0,
parameter APU_NARGS_CPU = 3,
parameter APU_WOP_CPU = 6,
parameter APU_NDSFLAGS_CPU = 15,
parameter APU_NUSFLAGS_CPU = 5,
parameter DM_HaltAddress = 32'h1A110800
parameter PULP_ZFINX = 0,
parameter DM_HALTADDRESS = 32'h1A110800
)
(
// Clock and Reset
Expand All @@ -75,11 +58,11 @@ module riscv_core
input logic [ 5:0] cluster_id_i,

// Instruction memory interface
output logic instr_req_o,
input logic instr_gnt_i,
input logic instr_rvalid_i,
output logic [31:0] instr_addr_o,
input logic [INSTR_RDATA_WIDTH-1:0] instr_rdata_i,
output logic instr_req_o,
input logic instr_gnt_i,
input logic instr_rvalid_i,
output logic [31:0] instr_addr_o,
input logic [31:0] instr_rdata_i,
Comment on lines +64 to +65
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Data memory interface
output logic data_req_o,
Expand All @@ -91,8 +74,6 @@ module riscv_core
output logic [31:0] data_wdata_o,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

input logic [31:0] data_rdata_i,

output logic [5:0] data_atop_o, // atomic operation, only active if parameter `A_EXTENSION != 0`

// apu-interconnect
// handshake signals
output logic apu_master_req_o,
Expand All @@ -111,7 +92,6 @@ module riscv_core
// Interrupt inputs
output logic irq_ack_o,
output logic [4:0] irq_id_o,
input logic irq_sec_i,

input logic irq_software_i,
input logic irq_timer_i,
Expand All @@ -120,23 +100,43 @@ module riscv_core
input logic irq_nmi_i,
input logic [31:0] irq_fastx_i,

output logic sec_lvl_o,

// Debug Interface
input logic debug_req_i,


// CPU Control Signals
input logic fetch_enable_i,
output logic core_busy_o,

input logic [N_EXT_PERF_COUNTERS-1:0] ext_perf_counters_i
output logic core_busy_o
);

// Unused parameters and signals (left in code for future design extensions)
localparam N_EXT_PERF_COUNTERS = 0;
localparam INSTR_RDATA_WIDTH = 32;
localparam PULP_SECURE = 0;
localparam N_PMP_ENTRIES = 16;
localparam USE_PMP = 0; // if PULP_SECURE is 1, you can still not use the PMP
localparam A_EXTENSION = 0;
localparam FP_DIVSQRT = FPU;
localparam SHARED_FP = 0;
localparam SHARED_DSP_MULT = 0;
localparam SHARED_INT_MULT = 0;
localparam SHARED_INT_DIV = 0;
localparam SHARED_FP_DIVSQRT = 0;

// Unused signals related to above unused parameters
// Left in code (with their original _i, _o postfixes) for future design extensions;
// these used to be former inputs/outputs of RI5CY

logic [5:0] data_atop_o; // atomic operation, only active if parameter `A_EXTENSION != 0`
logic [N_EXT_PERF_COUNTERS-1:0] ext_perf_counters_i = 'b0;
logic irq_sec_i = 1'b0;
logic sec_lvl_o;

localparam N_HWLP = 2;
localparam N_HWLP_BITS = $clog2(N_HWLP);
localparam APU = ((SHARED_DSP_MULT==1) | (SHARED_INT_DIV==1) | (FPU==1)) ? 1 : 0;


// IF/ID signals
logic is_hwlp_id;
logic [N_HWLP-1:0] hwlp_dec_cnt_id;
Expand Down Expand Up @@ -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 )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)
if_stage_i
(
Expand Down Expand Up @@ -561,7 +561,7 @@ module riscv_core
.A_EXTENSION ( A_EXTENSION ),
.APU ( APU ),
.FPU ( FPU ),
.Zfinx ( Zfinx ),
.PULP_ZFINX ( PULP_ZFINX ),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dito.

.FP_DIVSQRT ( FP_DIVSQRT ),
.SHARED_FP ( SHARED_FP ),
.SHARED_DSP_MULT ( SHARED_DSP_MULT ),
Expand Down
6 changes: 3 additions & 3 deletions rtl/riscv_id_stage.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ module riscv_id_stage
parameter A_EXTENSION = 0,
parameter APU = 0,
parameter FPU = 0,
parameter Zfinx = 0,
parameter PULP_ZFINX = 0,
parameter FP_DIVSQRT = 0,
parameter SHARED_FP = 0,
parameter SHARED_DSP_MULT = 0,
Expand Down Expand Up @@ -497,7 +497,7 @@ module riscv_id_stage
//-- FPU Register file enable:
//-- Taken from Cluster Config Reg if FPU reg file exists, or always disabled
//-----------------------------------------------------------------------------
assign fregfile_ena = FPU && !Zfinx ? ~fregfile_disable_i : '0;
assign fregfile_ena = FPU && !PULP_ZFINX ? ~fregfile_disable_i : '0;

//---------------------------------------------------------------------------
// source register selection regfile_fp_x=1 <=> REG_x is a FP-register
Expand Down Expand Up @@ -973,7 +973,7 @@ module riscv_id_stage
#(
.ADDR_WIDTH(6),
.FPU(FPU),
.Zfinx(Zfinx)
.PULP_ZFINX(PULP_ZFINX)
)
registers_i
(
Expand Down
4 changes: 2 additions & 2 deletions rtl/riscv_if_stage.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ module riscv_if_stage
parameter N_HWLP = 2,
parameter RDATA_WIDTH = 32,
parameter FPU = 0,
parameter DM_HaltAddress = 32'h1A110800
parameter DM_HALTADDRESS = 32'h1A110800
)
(
input logic clk,
Expand Down Expand Up @@ -142,7 +142,7 @@ module riscv_if_stage
unique case (exc_pc_mux_i)
EXC_PC_EXCEPTION: exc_pc = { trap_base_addr, 8'h0 }; //1.10 all the exceptions go to base address
EXC_PC_IRQ: exc_pc = { trap_base_addr, 1'b0, exc_vec_pc_mux_i[4:0], 2'b0 }; // interrupts are vectored
EXC_PC_DBD: exc_pc = { DM_HaltAddress };
EXC_PC_DBD: exc_pc = { DM_HALTADDRESS };
default:;
endcase
end
Expand Down
12 changes: 6 additions & 6 deletions rtl/riscv_register_file.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@
// Description: Register file with 31x 32 bit wide registers. Register 0 //
// is fixed to 0. This register file is based on flip-flops. //
// Also supports the fp-register file now if FPU=1 //
// If Zfinx is 1, floating point operations take values from //
// the X register file //
// If PULP_ZFINX is 1, floating point operations take values //
// from the X register file //
// //
////////////////////////////////////////////////////////////////////////////////

Expand All @@ -32,7 +32,7 @@ module riscv_register_file
parameter ADDR_WIDTH = 5,
parameter DATA_WIDTH = 32,
parameter FPU = 0,
parameter Zfinx = 0
parameter PULP_ZFINX = 0
)
(
// Clock and Reset
Expand Down Expand Up @@ -68,7 +68,7 @@ module riscv_register_file
localparam NUM_WORDS = 2**(ADDR_WIDTH-1);
// number of floating point registers
localparam NUM_FP_WORDS = 2**(ADDR_WIDTH-1);
localparam NUM_TOT_WORDS = FPU ? ( Zfinx ? NUM_WORDS : NUM_WORDS + NUM_FP_WORDS ) : NUM_WORDS;
localparam NUM_TOT_WORDS = FPU ? ( PULP_ZFINX ? NUM_WORDS : NUM_WORDS + NUM_FP_WORDS ) : NUM_WORDS;

// integer register file
logic [NUM_WORDS-1:0][DATA_WIDTH-1:0] mem;
Expand All @@ -88,7 +88,7 @@ module riscv_register_file
//-----------------------------------------------------------------------------
//-- READ : Read address decoder RAD
//-----------------------------------------------------------------------------
if (FPU == 1 && Zfinx == 0) begin
if (FPU == 1 && PULP_ZFINX == 0) begin
assign rdata_a_o = raddr_a_i[5] ? mem_fp[raddr_a_i[4:0]] : mem[raddr_a_i[4:0]];
assign rdata_b_o = raddr_b_i[5] ? mem_fp[raddr_b_i[4:0]] : mem[raddr_b_i[4:0]];
assign rdata_c_o = raddr_c_i[5] ? mem_fp[raddr_c_i[4:0]] : mem[raddr_c_i[4:0]];
Expand Down Expand Up @@ -161,7 +161,7 @@ module riscv_register_file

end

if (FPU == 1 && Zfinx == 0) begin
if (FPU == 1 && PULP_ZFINX == 0) begin
// Floating point registers
for(l = 0; l < NUM_FP_WORDS; l++) begin
always_ff @(posedge clk, negedge rst_n)
Expand Down
12 changes: 6 additions & 6 deletions rtl/riscv_register_file_latch.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@
// is fixed to 0. This register file is based on latches and //
// is thus smaller than the flip-flop based register file. //
// Also supports the fp-register file now if FPU=1 //
// If Zfinx is 1, floating point operations take values from //
// the X register file //
// If PULP_ZFINX is 1, floating point operations take values //
// from the X register file //
// //
////////////////////////////////////////////////////////////////////////////////

Expand All @@ -34,7 +34,7 @@ module riscv_register_file
parameter ADDR_WIDTH = 5,
parameter DATA_WIDTH = 32,
parameter FPU = 0,
parameter Zfinx = 0
parameter PULP_ZFINX = 0
)
(
// Clock and Reset
Expand Down Expand Up @@ -70,7 +70,7 @@ module riscv_register_file
localparam NUM_WORDS = 2**(ADDR_WIDTH-1);
// number of floating point registers
localparam NUM_FP_WORDS = 2**(ADDR_WIDTH-1);
localparam NUM_TOT_WORDS = FPU ? ( Zfinx ? NUM_WORDS : NUM_WORDS + NUM_FP_WORDS ) : NUM_WORDS;
localparam NUM_TOT_WORDS = FPU ? ( PULP_ZFINX ? NUM_WORDS : NUM_WORDS + NUM_FP_WORDS ) : NUM_WORDS;

// integer register file
logic [DATA_WIDTH-1:0] mem[NUM_WORDS];
Expand Down Expand Up @@ -100,7 +100,7 @@ module riscv_register_file
//-----------------------------------------------------------------------------
//-- READ : Read address decoder RAD
//-----------------------------------------------------------------------------
if (FPU == 1 && Zfinx == 0) begin
if (FPU == 1 && PULP_ZFINX == 0) begin
assign rdata_a_o = raddr_a_i[5] ? mem_fp[raddr_a_i[4:0]] : mem[raddr_a_i[4:0]];
assign rdata_b_o = raddr_b_i[5] ? mem_fp[raddr_b_i[4:0]] : mem[raddr_b_i[4:0]];
assign rdata_c_o = raddr_c_i[5] ? mem_fp[raddr_c_i[4:0]] : mem[raddr_c_i[4:0]];
Expand Down Expand Up @@ -209,7 +209,7 @@ module riscv_register_file
end
end

if (FPU == 1 && Zfinx == 0) begin
if (FPU == 1 && PULP_ZFINX == 0) begin
// Floating point registers
always_latch
begin : latch_wdata_fp
Expand Down
5 changes: 2 additions & 3 deletions tb/core/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,7 @@ FIRMWARE_OBJS = $(addprefix firmware/, start.o \
FIRMWARE_TEST_OBJS = $(addsuffix .o, \
$(basename $(wildcard riscv_tests/rv32ui/*.S)) \
$(basename $(wildcard riscv_tests/rv32um/*.S)) \
$(basename $(wildcard riscv_tests/rv32uc/*.S)) \
$(basename $(wildcard riscv_tests/rv32ua/*.S)))
$(basename $(wildcard riscv_tests/rv32uc/*.S)))

COMPLIANCE_TEST_OBJS = $(addsuffix .o, \
$(basename $(wildcard riscv_compliance_tests/*.S)))
Expand Down Expand Up @@ -371,7 +370,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 $@ \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good

$(RISCV_TEST_INCLUDES) \
-DTEST_FUNC_NAME=$(notdir $(basename $<)) \
-DTEST_FUNC_TXT='"$(notdir $(basename $<))"' \
Expand Down
11 changes: 0 additions & 11 deletions tb/core/firmware/start.S
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,17 +317,6 @@ start:

TEST(fence_i)

TEST(amoadd_w)
TEST(amoand_w)
TEST(amomaxu_w)
TEST(amomax_w)
TEST(amominu_w)
TEST(amomin_w)
TEST(amoor_w)
TEST(amoswap_w)
TEST(amoxor_w)
TEST(lrsc)

Copy link
Contributor

@bluewww bluewww Apr 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/* running riscv-compliance-tests */
la a0, riscv_compliance_tests_msg
call print_str
Expand Down
10 changes: 6 additions & 4 deletions tb/core/mm_ram.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -698,7 +698,8 @@ module mm_ram

`ifndef VERILATOR
riscv_random_stall
#(.DATA_WIDTH(INSTR_RDATA_WIDTH))
#(.ADDR_WIDTH(RAM_ADDR_WIDTH),
.DATA_WIDTH(INSTR_RDATA_WIDTH))
instr_random_stalls
(
.clk_i ( clk_i ),
Expand All @@ -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) ),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this will generate a port mismatch warning

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

.addr_core_i ( instr_addr_i ),
.addr_mem_o ( rnd_stall_instr_addr ),

.wdata_core_i ( ),
Expand All @@ -734,7 +735,8 @@ module mm_ram
);

riscv_random_stall
#(.DATA_WIDTH(32))
#(.ADDR_WIDTH(RAM_ADDR_WIDTH),
.DATA_WIDTH(32))
data_random_stalls
(
.clk_i ( clk_i ),
Expand All @@ -751,7 +753,7 @@ module mm_ram
.req_core_i ( data_req_dec ),
.req_mem_o ( rnd_stall_data_req ),

.addr_core_i ( 32'(data_addr_dec) ),
.addr_core_i ( data_addr_dec ),
.addr_mem_o ( rnd_stall_data_addr ),

.wdata_core_i ( data_wdata_dec ),
Expand Down
Loading