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.

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

M-extension support for SERV #60

Merged
merged 33 commits into from
Aug 20, 2021
Merged

M-extension support for SERV #60

merged 33 commits into from
Aug 20, 2021

Conversation

zeeshanrafique23
Copy link
Contributor

@zeeshanrafique23 zeeshanrafique23 commented Aug 17, 2021

This project was the part of GSoC'21. Completed by @zeeshanrafique23 under the mentorship of @olofk.

The RTL of M-extension and changes made in serv are verified with RISC-V compliance tests. Follow the README to run the tests.

The parent repository of MDU can be found here: https://github.com/zeeshanrafique23/mdu.git

To run the serv with M-extension you have to add mdu in fusesoc core list, run this command to check the existing core:

fusesoc core list

and to add mdu as a fusesoc core run this command:

fusesoc library add mdu https://github.com/zeeshanrafique23/mdu.git

Now run this command to build the serv core with mdu:

fusesoc run --target=verilator_tb --flag=mdu servant

Add --vcd flag to generate .vcd file to see the waveforms.

For more details check this Blog.

Copy link
Owner

@olofk olofk left a comment

Choose a reason for hiding this comment

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

Looks good! Most of the changes are just renaming and a little cleaning up.

@@ -17,11 +19,13 @@ module serv_bufreg
input wire i_imm,
output wire o_q,
//External
output wire [31:0] o_dbus_adr);
output wire [31:0] o_dbus_adr,
output wire [31:0] o_mdu_rs1);
Copy link
Owner

Choose a reason for hiding this comment

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

Rename o_mdu_rs1 to o_ext_rs1 instead since this can potentially be shared between different extensions

@@ -17,6 +18,8 @@ module serv_decode #(
output reg o_shift_op,
output reg o_slt_op,
output reg o_rd_op,
output reg o_mdu_op,
output reg [2:0] o_mdu_opcode,
Copy link
Owner

Choose a reason for hiding this comment

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

Let's rename o_mdu_opcode to o_ext_funct3 to show what the value comes from and that the signal can be useful for multiple extensions


generate
if(MDU) begin
wire mdu_rd = i_mdu_op & (dat_valid ? dat_cur : signbit & i_signed);
Copy link
Owner

Choose a reason for hiding this comment

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

I think that dat_valid will always be raised for MDU operations. Please check if we can simplify this expression to just i_mdu_op & dat_cur

rtl/serv_rf_top.v Show resolved Hide resolved
rtl/serv_rf_top.v Show resolved Hide resolved
rtl/serv_top.v Outdated

wire [4:0] rd_addr;
wire [4:0] rs1_addr;
wire [4:0] rs2_addr;

wire [3:0] immdec_ctrl;
wire [3:0] immdec_en;

Copy link
Owner

Choose a reason for hiding this comment

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

Remove added whitespace

rtl/serv_top.v Outdated
@@ -386,7 +405,6 @@ module serv_top
.i_csr_rd (csr_rd),
.i_rd_csr_en (rd_csr_en),
.i_mem_rd (mem_rd),

Copy link
Owner

Choose a reason for hiding this comment

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

Restore removed line

rtl/serv_top.v Show resolved Hide resolved
serv.core Outdated
@@ -43,6 +44,11 @@ targets:
toplevel : serv_rf_top

parameters:
MDU:
datatype : int
description: Enables RISC-V standard M-extension
Copy link
Owner

Choose a reason for hiding this comment

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

Enables interface for RISC-V standard M-extension

servant.core Outdated
@@ -39,7 +39,7 @@ filesets:
- "!tool_quartus? (servant/servant_ram.v)"
- servant/servant.v
file_type : verilogSource
depend : [serv]
depend : {serv, mdu}
Copy link
Owner

Choose a reason for hiding this comment

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

Use [ ] instead of { }. I actually didn't know it was allowed to use { }. Seems to work fine but made me a bit confused :)

@zeeshanrafique23
Copy link
Contributor Author

Thanks for reviewing, the code is modified according to the requested changes.

Copy link
Owner

@olofk olofk left a comment

Choose a reason for hiding this comment

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

Close now, but I missed one thing from my first review

servant.core Outdated
@@ -347,6 +347,7 @@ targets:
filesets : [soc, servant_tb]
parameters :
- RISCV_FORMAL
- MDU
Copy link
Owner

Choose a reason for hiding this comment

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

This should also be "- mdu? (MDU=1)"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, this should be.

assign dbus_ack = i_dbus_ack;
end
assign o_ext_rs2 = o_dbus_dat;
endgenerate
Copy link
Owner

Choose a reason for hiding this comment

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

I just realized that we need to move this generate block inside serv_top.v so that we can expose the extension interface for designs that uses serv_top directly instead of serv_rf_top (e.g. serving)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

@olofk olofk merged commit 7ea0864 into olofk:main Aug 20, 2021
@olofk
Copy link
Owner

olofk commented Aug 20, 2021

Awesome! I have nothing more to complain about now. :) Well done and thank you for your contributions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants