From 95f023208ecaa516860e66a1701a93912ffc62a2 Mon Sep 17 00:00:00 2001 From: mbertuletti Date: Wed, 4 Jun 2025 13:03:55 +0200 Subject: [PATCH 1/6] Present valid request only on WriteBack state --- src/obi_atop_resolver.sv | 1 + 1 file changed, 1 insertion(+) diff --git a/src/obi_atop_resolver.sv b/src/obi_atop_resolver.sv index d5c1581..dcd286c 100644 --- a/src/obi_atop_resolver.sv +++ b/src/obi_atop_resolver.sv @@ -378,6 +378,7 @@ module obi_atop_resolver mgr_port_req_o.a.be = '0; // serve from register if we cut the path if (RegisterAmo) begin + mgr_port_req_o.req = (state_q == WriteBackAMO); mgr_port_req_o.a.wdata = amo_result_q; mgr_port_req_o.a.be = {RiscvWordWidth/8{1'b1}} << (amo_operand_addr_q * RiscvWordWidth/8); From 42c635526593cc9353c7adba62275d1919015d87 Mon Sep 17 00:00:00 2001 From: mbertuletti Date: Wed, 11 Jun 2025 09:53:21 +0200 Subject: [PATCH 2/6] Remove non-resettable flops --- src/obi_atop_resolver.sv | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/obi_atop_resolver.sv b/src/obi_atop_resolver.sv index dcd286c..9c5f1ad 100644 --- a/src/obi_atop_resolver.sv +++ b/src/obi_atop_resolver.sv @@ -393,8 +393,8 @@ module obi_atop_resolver end if (RegisterAmo) begin : gen_amo_slice - `FFLNR(amo_result_q, amo_result, (state_q == DoAMO), clk_i) - `FFLNR(amo_operand_addr_q, amo_operand_addr, (state_q == DoAMO), clk_i) + `FFL(amo_result_q, amo_result, (state_q == DoAMO), '0, clk_i, rst_ni) + `FFL(amo_operand_addr_q, amo_operand_addr, (state_q == DoAMO), '0, clk_i, rst_ni) end else begin : gen_amo_slice assign amo_result_q = '0; assign amo_operand_addr_q = '0; From 8ceb36dd8b1a2179d56cc7ddb0c31d6b11257a28 Mon Sep 17 00:00:00 2001 From: Tim Fischer Date: Fri, 27 Jun 2025 13:30:43 +0200 Subject: [PATCH 3/6] atop_resolver: Support outstanding transactions --- src/obi_atop_resolver.sv | 91 +++++++++++++++++++++++++++------------- 1 file changed, 61 insertions(+), 30 deletions(-) diff --git a/src/obi_atop_resolver.sv b/src/obi_atop_resolver.sv index 9c5f1ad..c527266 100644 --- a/src/obi_atop_resolver.sv +++ b/src/obi_atop_resolver.sv @@ -34,7 +34,9 @@ module obi_atop_resolver // Word width of the widest RISC-V processor that can issue requests to this module. // 32 for RV32; 64 for RV64, where both 32-bit (.W suffix) and 64-bit (.D suffix) AMOs are // supported if `aw_strb` is set correctly. - parameter int unsigned RiscvWordWidth = 32 + parameter int unsigned RiscvWordWidth = 32, + /// Number of outstanding transactions. Only relevant if downstream interface is cut + parameter int unsigned NumTxns = 1 ) ( input logic clk_i, input logic rst_ni, @@ -70,8 +72,9 @@ module obi_atop_resolver amo_state_e state_q, state_d; logic load_amo; - obi_atop_e amo_op_q; + obi_atop_e amo_op_d, amo_op_q; logic amo_wb; + logic amo_available, amo_last; logic [SbrPortObiCfg.AddrWidth-1:0] addr_q; logic [ SbrPortObiCfg.IdWidth-1:0] aid_q; @@ -334,8 +337,8 @@ module obi_atop_resolver always_comb begin // feed-through - sbr_port_rsp_o.gnt = rdata_ready & mgr_port_rsp_i.gnt; - mgr_port_req_o.req = sbr_port_req_i.req & rdata_ready; + sbr_port_rsp_o.gnt = rdata_ready & mgr_port_rsp_i.gnt & amo_available; + mgr_port_req_o.req = sbr_port_req_i.req & rdata_ready & amo_available; mgr_port_req_o.a.addr = sbr_port_req_i.a.addr; mgr_port_req_o.a.we = obi_atop_e'(sbr_port_req_i.a.a_optional.atop) != ATOPSC ? sbr_port_req_i.a.we : sc_successful_or_lr_d; @@ -347,6 +350,7 @@ module obi_atop_resolver state_d = state_q; load_amo = 1'b0; amo_wb = 1'b0; + amo_op_d = amo_op_q; unique case (state_q) Idle: begin @@ -355,6 +359,7 @@ module obi_atop_resolver !((obi_atop_e'(sbr_port_req_i.a.a_optional.atop) inside {ATOPLR, ATOPSC}) || !sbr_port_req_i.a.a_optional.atop[5])) begin load_amo = 1'b1; + amo_op_d = obi_atop_e'(sbr_port_req_i.a.a_optional.atop); state_d = DoAMO; if (obi_atop_e'(sbr_port_req_i.a.a_optional.atop) inside {AMOSWAP, AMOADD, AMOXOR, AMOAND, AMOOR, AMOMIN, AMOMAX, @@ -362,44 +367,69 @@ module obi_atop_resolver mgr_port_req_o.a.we = 1'b0; end end + end - // Claim the memory interface DoAMO, WriteBackAMO: begin sbr_port_rsp_o.gnt = 1'b0; - if (mgr_port_rsp_i.gnt) begin - state_d = (RegisterAmo && state_q != WriteBackAMO) ? WriteBackAMO : Idle; - end - // Commit AMO - amo_wb = 1'b1; - mgr_port_req_o.req = 1'b1; - mgr_port_req_o.a.we = 1'b1; - mgr_port_req_o.a.addr = addr_q; - mgr_port_req_o.a.aid = aid_q; - mgr_port_req_o.a.be = '0; - // serve from register if we cut the path - if (RegisterAmo) begin - mgr_port_req_o.req = (state_q == WriteBackAMO); - mgr_port_req_o.a.wdata = amo_result_q; - mgr_port_req_o.a.be = {RiscvWordWidth/8{1'b1}} << - (amo_operand_addr_q * RiscvWordWidth/8); - end else begin - mgr_port_req_o.a.wdata = amo_result; - mgr_port_req_o.a.be = {RiscvWordWidth/8{1'b1}} << - (amo_operand_addr * RiscvWordWidth/8); + mgr_port_req_o.req = 1'b0; + if (amo_last && mgr_port_rsp_i.rvalid) begin + if (mgr_port_rsp_i.gnt) begin + state_d = (RegisterAmo && state_q != WriteBackAMO) ? WriteBackAMO : Idle; + end + // Commit AMO + amo_op_d = ATOPNONE; + amo_wb = 1'b1; + mgr_port_req_o.req = 1'b1; + mgr_port_req_o.a.we = 1'b1; + mgr_port_req_o.a.addr = addr_q; + mgr_port_req_o.a.aid = aid_q; + mgr_port_req_o.a.be = '0; + // serve from register if we cut the path + if (RegisterAmo) begin + mgr_port_req_o.req = (state_q == WriteBackAMO); + mgr_port_req_o.a.wdata = amo_result_q; + mgr_port_req_o.a.be = {RiscvWordWidth/8{1'b1}} << + (amo_operand_addr_q * RiscvWordWidth/8); + end else begin + mgr_port_req_o.a.wdata = amo_result; + mgr_port_req_o.a.be = {RiscvWordWidth/8{1'b1}} << + (amo_operand_addr * RiscvWordWidth/8); + end end end - default: ; + default:; endcase end if (RegisterAmo) begin : gen_amo_slice - `FFL(amo_result_q, amo_result, (state_q == DoAMO), '0, clk_i, rst_ni) - `FFL(amo_operand_addr_q, amo_operand_addr, (state_q == DoAMO), '0, clk_i, rst_ni) + `FFLNR(amo_result_q, amo_result, (state_q == DoAMO), clk_i) + `FFLNR(amo_operand_addr_q, amo_operand_addr, (state_q == DoAMO), clk_i) end else begin : gen_amo_slice assign amo_result_q = '0; assign amo_operand_addr_q = '0; end + logic rsp_happening; + if (SbrPortObiCfg.UseRReady) begin : gen_rsp_happening + assign rsp_happening = mgr_port_rsp_i.rvalid & mgr_port_req_o.rready; + end else begin : gen_rsp_norready + assign rsp_happening = mgr_port_rsp_i.rvalid; + end + + credit_counter #( + .NumCredits (NumTxns) + ) i_credit_counter ( + .clk_i, + .rst_ni, + .credit_o (), + .credit_give_i(rsp_happening), + .credit_take_i(mgr_port_req_o.req & mgr_port_rsp_i.gnt), + .credit_init_i('0), + .credit_left_o(amo_available), + .credit_crit_o(amo_last), + .credit_full_o() + ); + always_ff @(posedge clk_i or negedge rst_ni) begin if (!rst_ni) begin state_q <= Idle; @@ -410,14 +440,15 @@ module obi_atop_resolver aid_q <= '0; end else begin state_q <= state_d; + amo_op_q <= amo_op_d; if (load_amo) begin - amo_op_q <= obi_atop_e'(sbr_port_req_i.a.a_optional.atop); + // amo_op_q <= obi_atop_e'(sbr_port_req_i.a.a_optional.atop); addr_q <= sbr_port_req_i.a.addr; be_q <= sbr_port_req_i.a.be; aid_q <= sbr_port_req_i.a.aid; amo_operand_b_q <= sbr_port_req_i.a.wdata; end else begin - amo_op_q <= ATOPNONE; + // amo_op_q <= ATOPNONE; end end end From c3bbe8f39e5ee1e60351d678d1ed1fdcea4803f7 Mon Sep 17 00:00:00 2001 From: Lorenzo Leone Date: Wed, 23 Jul 2025 18:04:04 +0200 Subject: [PATCH 4/6] Add fifo metadata for out transactions --- src/obi_atop_resolver.sv | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/obi_atop_resolver.sv b/src/obi_atop_resolver.sv index c527266..e37e7a4 100644 --- a/src/obi_atop_resolver.sv +++ b/src/obi_atop_resolver.sv @@ -102,13 +102,17 @@ module obi_atop_resolver ); // Store the metadata at handshake - spill_register #( - .T (logic [SbrPortObiCfg.IdWidth-1:0]), - .Bypass(1'b0) + stream_fifo #( + .T (logic [SbrPortObiCfg.IdWidth-1:0]), + .DEPTH (NumTxns), + .FALL_THROUGH (1'b0) ) i_metadata_register ( .clk_i, .rst_ni, - .valid_i(sbr_port_req_i.req && sbr_port_rsp_o.gnt), + .flush_i ('0), + .testmode_i ('0), + .usage_o (), + .valid_i (sbr_port_req_i.req && sbr_port_rsp_o.gnt), .ready_o(meta_ready), .data_i (sbr_port_req_i.a.aid), .valid_o(meta_valid), From f81aa0c3206454fd52da12d502e82b457e9e4cc6 Mon Sep 17 00:00:00 2001 From: Lorenzo Leone Date: Thu, 24 Jul 2025 19:35:26 +0200 Subject: [PATCH 5/6] hw: Fix multiple outstanding ATOP misbehaviour --- src/obi_atop_resolver.sv | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/obi_atop_resolver.sv b/src/obi_atop_resolver.sv index e37e7a4..5716dfd 100644 --- a/src/obi_atop_resolver.sv +++ b/src/obi_atop_resolver.sv @@ -74,6 +74,7 @@ module obi_atop_resolver logic load_amo; obi_atop_e amo_op_d, amo_op_q; logic amo_wb; + logic rsp_happening; logic amo_available, amo_last; logic [SbrPortObiCfg.AddrWidth-1:0] addr_q; @@ -124,7 +125,7 @@ module obi_atop_resolver logic rdata_full, rdata_empty; logic rdata_usage; - assign rdata_ready = !rdata_usage && !rdata_full; + assign rdata_ready = !rdata_full; assign rdata_valid = !rdata_empty; logic sc_successful_or_lr_d, sc_successful_or_lr_q; @@ -170,11 +171,11 @@ module obi_atop_resolver .flush_i(1'b0), .full_o (rdata_full), .empty_o(rdata_empty), - .usage_o(rdata_usage), + .usage_o( ), .data_i (out_buf_fifo_in), - .push_i (~last_amo_wb & mgr_port_rsp_i.rvalid), + .push_i (~last_amo_wb & rsp_happening), .data_o (out_buf_fifo_out), - .pop_i (pop_resp & ~rdata_empty) + .pop_i (pop_resp) ); // In case of a SC we must forward SC result from the cycle earlier. @@ -184,16 +185,16 @@ module obi_atop_resolver // Ready to output data if both meta and read data // are available (the read data will always be last) - assign sbr_port_rsp_o.rvalid = meta_valid & rdata_valid; + assign sbr_port_rsp_o.rvalid = meta_valid & rdata_valid & ~amo_wb; // Only pop the data from the registers once both registers are ready if (SbrPortObiCfg.UseRReady) begin : gen_pop_rready assign pop_resp = sbr_port_rsp_o.rvalid & sbr_port_req_i.rready; end else begin : gen_pop_norready - assign pop_resp = sbr_port_rsp_o.rvalid; + assign pop_resp = sbr_port_rsp_o.rvalid; // TODO (lleone): Rvalid in case of AMO must be asserted only after the read? end // Buffer amo_wb signal to ensure wb rdata is not used - `FFL(last_amo_wb, amo_wb, mgr_port_req_o.req, 1'b0, clk_i, rst_ni); + `FF(last_amo_wb, amo_wb, 1'b0, clk_i, rst_ni); // ---------------- // LR/SC @@ -339,6 +340,12 @@ module obi_atop_resolver assign a_optional = '0; end + if (SbrPortObiCfg.UseRReady) begin : gen_rsp_happening + assign rsp_happening = mgr_port_rsp_i.rvalid & mgr_port_req_o.rready; + end else begin : gen_rsp_norready + assign rsp_happening = mgr_port_rsp_i.rvalid; + end + always_comb begin // feed-through sbr_port_rsp_o.gnt = rdata_ready & mgr_port_rsp_i.gnt & amo_available; @@ -353,7 +360,7 @@ module obi_atop_resolver state_d = state_q; load_amo = 1'b0; - amo_wb = 1'b0; + amo_wb = last_amo_wb & ~rsp_happening; amo_op_d = amo_op_q; unique case (state_q) @@ -376,7 +383,7 @@ module obi_atop_resolver DoAMO, WriteBackAMO: begin sbr_port_rsp_o.gnt = 1'b0; mgr_port_req_o.req = 1'b0; - if (amo_last && mgr_port_rsp_i.rvalid) begin + if (amo_last && rsp_happening) begin if (mgr_port_rsp_i.gnt) begin state_d = (RegisterAmo && state_q != WriteBackAMO) ? WriteBackAMO : Idle; end @@ -413,13 +420,6 @@ module obi_atop_resolver assign amo_operand_addr_q = '0; end - logic rsp_happening; - if (SbrPortObiCfg.UseRReady) begin : gen_rsp_happening - assign rsp_happening = mgr_port_rsp_i.rvalid & mgr_port_req_o.rready; - end else begin : gen_rsp_norready - assign rsp_happening = mgr_port_rsp_i.rvalid; - end - credit_counter #( .NumCredits (NumTxns) ) i_credit_counter ( From acfcd0f80c7539aa8da7821a66d9acf2074a5b4e Mon Sep 17 00:00:00 2001 From: Lorenzo Leone Date: Thu, 24 Jul 2025 20:25:55 +0200 Subject: [PATCH 6/6] lint: Fix linting checks --- src/obi_atop_resolver.sv | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/obi_atop_resolver.sv b/src/obi_atop_resolver.sv index 5716dfd..357eb46 100644 --- a/src/obi_atop_resolver.sv +++ b/src/obi_atop_resolver.sv @@ -190,7 +190,7 @@ module obi_atop_resolver if (SbrPortObiCfg.UseRReady) begin : gen_pop_rready assign pop_resp = sbr_port_rsp_o.rvalid & sbr_port_req_i.rready; end else begin : gen_pop_norready - assign pop_resp = sbr_port_rsp_o.rvalid; // TODO (lleone): Rvalid in case of AMO must be asserted only after the read? + assign pop_resp = sbr_port_rsp_o.rvalid; end // Buffer amo_wb signal to ensure wb rdata is not used