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

[regfile_fpga] oh_raddr_*_err signals unassigned if RdataMuxCheck=0 #2230

Closed
glaserf opened this issue Dec 6, 2024 · 2 comments
Closed

[regfile_fpga] oh_raddr_*_err signals unassigned if RdataMuxCheck=0 #2230

glaserf opened this issue Dec 6, 2024 · 2 comments
Labels

Comments

@glaserf
Copy link

glaserf commented Dec 6, 2024

Observed Behavior

The onehot-error signals in relation to the read addresses (oh_raddr_a_err, oh_raddr_b_err) in rtl/ibex_register_file_fpga.sv are unassigned for non-secure Ibex configurations (which is for example used for opentitan FPGA top-levels IIRC):

end else begin : gen_no_rdata_mux_check
// async_read a
assign rdata_a_o = (raddr_a_i == '0) ? WordZeroVal : mem[raddr_a_i];
// async_read b
assign rdata_b_o = (raddr_b_i == '0) ? WordZeroVal : mem[raddr_b_i];
end

This leads to the regfile err_o signal becoming x

assign err_o = oh_raddr_a_err || oh_raddr_b_err || oh_we_err;

which triggers assertions (at least for us) when simulating with the FPGA regfile. It however seems to not cause issues during synthesis, at least with Vivado.

I am also unsure about the comments that refer to rdata_*_o as asynchronous:

// async_read a
assign rdata_a_o = (raddr_a_i == '0) ? WordZeroVal : mem[raddr_a_i];

Expected Behavior

Same as for the other regfile variants; signals in question being tied to 1'b0 for RdataMuxCheck = 0.

@glaserf glaserf added the Type:Bug Bugs label Dec 6, 2024
@shareefj
Copy link

See #2241

@rswarbrick
Copy link
Contributor

Oh! Looks like it's fixed! Thanks for adding a note.

About the asynchronous comments, they date back to the original version of the file (from 2020). I think the author probably meant to point out that raddr_a_i feeds through to rdata_a_o without any register stage, so isn't synchronous to some clock. But it's rather confusingly phrased.

I've just checked, and the behaviour is the same for e.g. ibex_register_file_ff. I'll file a PR to drop the comment for the FPGA register file so that things are a bit less confusing.

rswarbrick added a commit to rswarbrick/ibex that referenced this issue Jan 23, 2025
These were noticed by someone responding to issue lowRISC#2230. I think the
author's original logic was to point out that there's a path from e.g.
raddr_a_i to rdata_a_o which doesn't depend on any clock, so is
"asynchronous".

But that's the same in the other modes and also for the other register
file implementations, which don't have analogous comments.

Drop these ones.
github-merge-queue bot pushed a commit that referenced this issue Jan 23, 2025
These were noticed by someone responding to issue #2230. I think the
author's original logic was to point out that there's a path from e.g.
raddr_a_i to rdata_a_o which doesn't depend on any clock, so is
"asynchronous".

But that's the same in the other modes and also for the other register
file implementations, which don't have analogous comments.

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

No branches or pull requests

3 participants