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

Add AW lock register to handle W FIFO push signal #2461

Merged
merged 3 commits into from
Sep 24, 2024

Conversation

ricted98
Copy link
Contributor

@ricted98 ricted98 commented Aug 24, 2024

The original implementation uses the AW handshake as the logical condition for a push into the W channel FIFO. However, this choice implicitly assume that the AW handshake will take place concurrently or before the W channel handshake.

This assumption is not mandatory in the AXI standard and a downstream IP can generate a situation where the W channel handshake precedes the AW channel one, resulting in a pop of a potentially empty FIFO.

Using an AW lock register to control the pushes on the FIFO makes sure that the proper W selection is stored as soon as the core generates a valid AW request (aw_valid = 1'b1) without relying on the occurrence of an AW handshake, which can happen out of order.

This specific situation was observed when using CVA6 with a downstream AXI ID serializer. Due to the serializer internal FIFOs being full, the AW request was gated but the W channel was passed through, thus generating the situation where the core observed aw_ready = 1'b0 and w_ready = 1'b1 and the empty W FIFO was popped.

);

always_ff @( posedge clk_i or negedge rst_ni ) begin : aw_lock_reg
Copy link
Contributor

Choose a reason for hiding this comment

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

[verible-verilog-format] reported by reviewdog 🐶

Suggested change
always_ff @( posedge clk_i or negedge rst_ni ) begin : aw_lock_reg
always_ff @(posedge clk_i or negedge rst_ni) begin : aw_lock_reg

);

always_ff @( posedge clk_i or negedge rst_ni ) begin : aw_lock_reg
if (~rst_ni) aw_lock_q <= 1'b0;
else aw_lock_q <= aw_lock_d;
Copy link
Contributor

Choose a reason for hiding this comment

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

[verible-verilog-format] reported by reviewdog 🐶

Suggested change
else aw_lock_q <= aw_lock_d;
else aw_lock_q <= aw_lock_d;

Comment on lines +213 to +214
assign w_fifo_pop = axi_req_o.w_valid & axi_resp_i.w_ready & axi_req_o.w.last;
assign aw_lock_d = ~axi_resp_i.aw_ready & (axi_req_o.aw_valid | aw_lock_q);
Copy link
Contributor

Choose a reason for hiding this comment

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

[verible-verilog-format] reported by reviewdog 🐶

Suggested change
assign w_fifo_pop = axi_req_o.w_valid & axi_resp_i.w_ready & axi_req_o.w.last;
assign aw_lock_d = ~axi_resp_i.aw_ready & (axi_req_o.aw_valid | aw_lock_q);
assign w_fifo_pop = axi_req_o.w_valid & axi_resp_i.w_ready & axi_req_o.w.last;
assign aw_lock_d = ~axi_resp_i.aw_ready & (axi_req_o.aw_valid | aw_lock_q);

@ricted98 ricted98 marked this pull request as ready for review August 24, 2024 08:57
Copy link
Contributor

✔️ successful run, report available here.

Copy link
Contributor

✔️ successful run, report available here.

Copy link
Contributor

👋 Hi there!

This pull request seems inactive. Need more help or have updates? Feel free to let us know. If there are no updates within the next few days, we'll go ahead and close this PR. 😊

@github-actions github-actions bot added the Status:Stale Issue or PR is stale and hasn't received any updates. label Sep 24, 2024
@JeanRochCoulon JeanRochCoulon removed the Status:Stale Issue or PR is stale and hasn't received any updates. label Sep 24, 2024
Copy link
Contributor

✔️ successful run, report available here.

@JeanRochCoulon JeanRochCoulon merged commit 164d7c7 into openhwgroup:master Sep 24, 2024
4 of 10 checks passed
ricted98 added a commit to pulp-platform/cva6 that referenced this pull request Oct 12, 2024
ricted98 added a commit to pulp-platform/cva6 that referenced this pull request Oct 12, 2024
@ricted98 ricted98 deleted the rt/fix-axi-write branch October 17, 2024 08:34
niwis pushed a commit to pulp-platform/cva6 that referenced this pull request Oct 31, 2024
niwis pushed a commit to pulp-platform/cva6 that referenced this pull request Oct 31, 2024
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