From fa3b663b6986a5c017b7942b936cdc5855beff57 Mon Sep 17 00:00:00 2001 From: Luca Rufer Date: Wed, 25 Feb 2026 18:05:25 +0100 Subject: [PATCH] atop_resolver: Fix handshaking and other bugs --- src/obi_atop_resolver.sv | 236 ++++++++++++++++++------------- src/test/tb_obi_atop_resolver.sv | 2 +- 2 files changed, 138 insertions(+), 100 deletions(-) diff --git a/src/obi_atop_resolver.sv b/src/obi_atop_resolver.sv index 357eb46..2f4a26b 100644 --- a/src/obi_atop_resolver.sv +++ b/src/obi_atop_resolver.sv @@ -30,13 +30,14 @@ module obi_atop_resolver /// Enable LR & SC AMOS parameter bit LrScEnable = 1, /// Cut path between request and response at the cost of increased AMO latency - parameter bit RegisterAmo = 1'b0, + /// Note: if RegisterAmo is 0, the ATOP resolver is not OBI protocol compliant + parameter bit RegisterAmo = 1'b1, // 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, /// Number of outstanding transactions. Only relevant if downstream interface is cut - parameter int unsigned NumTxns = 1 + parameter int unsigned NumTxns = 2 ) ( input logic clk_i, input logic rst_ni, @@ -61,21 +62,23 @@ module obi_atop_resolver logic [SbrPortObiCfg.DataWidth-1:0] out_rdata; logic pop_resp; - logic last_amo_wb; typedef enum logic [1:0] { Idle, - DoAMO, + WaitAMOLoad, WriteBackAMO } amo_state_e; amo_state_e state_q, state_d; + logic sc, exokay; + logic load_amo; + logic save_amo_result; + logic amo_ignore_rsp_d, amo_ignore_rsp_q; obi_atop_e amo_op_d, amo_op_q; - logic amo_wb; logic rsp_happening; - logic amo_available, amo_last; + logic amo_available, amo_last_outstanding; logic [SbrPortObiCfg.AddrWidth-1:0] addr_q; logic [ SbrPortObiCfg.IdWidth-1:0] aid_q; @@ -102,9 +105,23 @@ module obi_atop_resolver .empty_o( /*Unused*/) ); + typedef struct packed { + logic [SbrPortObiCfg.IdWidth-1:0] aid; + logic exokay; + logic sc; + } meta_buffer_t; + + meta_buffer_t meta_buf_fifo_in, meta_buf_fifo_out; + + assign meta_buf_fifo_in = '{ + aid: sbr_port_req_i.a.aid, + exokay: exokay, + sc: sc + }; + // Store the metadata at handshake stream_fifo #( - .T (logic [SbrPortObiCfg.IdWidth-1:0]), + .T (meta_buffer_t), .DEPTH (NumTxns), .FALL_THROUGH (1'b0) ) i_metadata_register ( @@ -113,47 +130,51 @@ module obi_atop_resolver .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), - .ready_i(pop_resp), - .data_o (sbr_port_rsp_o.r.rid) + .valid_i (sbr_port_req_i.req && sbr_port_rsp_o.gnt), + .ready_o (meta_ready), + .data_i (meta_buf_fifo_in), + .valid_o (meta_valid), + .ready_i (pop_resp), + .data_o (meta_buf_fifo_out) ); // Store response if it's not accepted immediately logic rdata_full, rdata_empty; - logic rdata_usage; assign rdata_ready = !rdata_full; assign rdata_valid = !rdata_empty; - logic sc_successful_or_lr_d, sc_successful_or_lr_q; - logic sc_q; - typedef struct packed { logic [SbrPortObiCfg.DataWidth-1:0] data; logic err; - logic exokay; mgr_port_obi_r_optional_t optional; } out_buffer_t; out_buffer_t out_buf_fifo_in, out_buf_fifo_out; + always_comb begin + out_rdata = mgr_port_rsp_i.r.rdata; + + // For an SC, set the rdata value according to the RISCV-spec + if (LrScEnable && meta_buf_fifo_out.sc) begin + out_rdata = meta_buf_fifo_out.exokay ? '0 : $unsigned(1); + end + end + assign out_buf_fifo_in = '{ data: out_rdata, err: mgr_port_rsp_i.r.err, - exokay: sc_successful_or_lr_q, optional: mgr_port_rsp_i.r.r_optional }; assign sbr_port_rsp_o.r.rdata = out_buf_fifo_out.data; + assign sbr_port_rsp_o.r.rid = meta_buf_fifo_out.aid; assign sbr_port_rsp_o.r.err = out_buf_fifo_out.err; - assign sbr_port_rsp_o.r.r_optional.exokay = out_buf_fifo_out.exokay; + assign sbr_port_rsp_o.r.r_optional.exokay = meta_buf_fifo_out.exokay; if (SbrPortObiCfg.OptionalCfg.RUserWidth) begin : gen_ruser if (MgrPortObiCfg.OptionalCfg.RUserWidth) begin : gen_ruser_assign always_comb begin sbr_port_rsp_o.r.r_optional.ruser = '0; - sbr_port_rsp_o.r.r_optional.ruser = out_buf_fifo_in.optional.ruser; + sbr_port_rsp_o.r.r_optional.ruser = out_buf_fifo_out.optional.ruser; end end else begin : gen_no_ruser assign sbr_port_rsp_o.r.r_optional.ruser = '0; @@ -173,19 +194,14 @@ module obi_atop_resolver .empty_o(rdata_empty), .usage_o( ), .data_i (out_buf_fifo_in), - .push_i (~last_amo_wb & rsp_happening), + .push_i (~amo_ignore_rsp_q & rsp_happening), .data_o (out_buf_fifo_out), .pop_i (pop_resp) ); - // In case of a SC we must forward SC result from the cycle earlier. - assign out_rdata = (sc_q && LrScEnable) ? $unsigned( - !sc_successful_or_lr_q - ) : mgr_port_rsp_i.r.rdata; - // 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 & ~amo_wb; + assign sbr_port_rsp_o.rvalid = meta_valid & rdata_valid; // 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; @@ -193,9 +209,6 @@ module obi_atop_resolver assign pop_resp = sbr_port_rsp_o.rvalid; end - // Buffer amo_wb signal to ensure wb rdata is not used - `FF(last_amo_wb, amo_wb, 1'b0, clk_i, rst_ni); - // ---------------- // LR/SC // ---------------- @@ -219,22 +232,22 @@ module obi_atop_resolver } reservation_t; reservation_t reservation_d, reservation_q; - `FF(sc_successful_or_lr_q, sc_successful_or_lr_d, 1'b0, clk_i, rst_ni); `FF(reservation_q, reservation_d, 1'b0, clk_i, rst_ni); - `FF(sc_q, - sbr_port_req_i.req & - sbr_port_rsp_o.gnt & - (obi_atop_e'(sbr_port_req_i.a.a_optional.atop) == ATOPSC), - 1'b0, clk_i, rst_ni); always_comb begin unique_requester_id = sbr_port_req_i.a.aid; reservation_d = reservation_q; - sc_successful_or_lr_d = 1'b0; + exokay = 1'b0; + sc = 1'b0; // new valid transaction if (sbr_port_req_i.req && sbr_port_rsp_o.gnt) begin + // Any LR (even if the reservation fails) will return exokay if reservations are supported. + if (obi_atop_e'(sbr_port_req_i.a.a_optional.atop) == ATOPLR) begin + exokay = 1'b1; + end + // An SC can only pair with the most recent LR in program order. // Place a reservation on the address if there isn't already a valid reservation. // We prevent a live-lock by don't throwing away the reservation of a hart unless @@ -244,7 +257,6 @@ module obi_atop_resolver reservation_d.valid = 1'b1; reservation_d.addr = sbr_port_req_i.a.addr; reservation_d.requester = unique_requester_id; - sc_successful_or_lr_d = 1'b1; end // An SC may succeed only if no store from another hart (or other device) to @@ -263,15 +275,25 @@ module obi_atop_resolver // An SC from the same hart clears any pending reservation. if (reservation_q.valid && obi_atop_e'(sbr_port_req_i.a.a_optional.atop) == ATOPSC && reservation_q.requester == unique_requester_id) begin - reservation_d.valid = 1'b0; - sc_successful_or_lr_d = (reservation_q.addr == sbr_port_req_i.a.addr); + reservation_d.valid = 1'b0; + // An SC success shall be signaled via exokay only if: + // - the target sending the response supports exclusive accesses, + // - the related reservation is still valid, and + // - the reservation set contains the bytes being written. + if (reservation_q.addr == sbr_port_req_i.a.addr) begin + exokay = 1'b1; + end + end + + if (obi_atop_e'(sbr_port_req_i.a.a_optional.atop) == ATOPSC) begin + sc = 1'b1; end + end end // always_comb - end else begin : gen_disable_lrcs - assign sc_q = 1'b0; - assign sc_successful_or_lr_d = 1'b0; - assign sc_successful_or_lr_q = 1'b0; + end else begin : gen_disable_lrsc + assign exokay = 1'b0; + assign sc = 1'b0; end // ---------------- @@ -340,7 +362,8 @@ module obi_atop_resolver assign a_optional = '0; end - if (SbrPortObiCfg.UseRReady) begin : gen_rsp_happening + if (MgrPortObiCfg.UseRReady) begin : gen_rsp_happening + assign mgr_port_req_o.rready = rdata_ready; 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; @@ -348,20 +371,32 @@ module obi_atop_resolver always_comb begin // feed-through - sbr_port_rsp_o.gnt = rdata_ready & mgr_port_rsp_i.gnt & amo_available; + sbr_port_rsp_o.gnt = rdata_ready & mgr_port_rsp_i.gnt & amo_available & meta_ready; 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; + mgr_port_req_o.a.we = sbr_port_req_i.a.we; mgr_port_req_o.a.wdata = sbr_port_req_i.a.wdata; mgr_port_req_o.a.be = sbr_port_req_i.a.be; mgr_port_req_o.a.aid = sbr_port_req_i.a.aid; mgr_port_req_o.a.a_optional = a_optional; + if (obi_atop_e'(sbr_port_req_i.a.a_optional.atop) inside {AMOSWAP, AMOADD, AMOXOR, AMOAND, + AMOOR, AMOMIN, AMOMAX, AMOMINU, + AMOMAXU}) begin + // For AMO read first, then modify and write later + mgr_port_req_o.a.we = 1'b0; + end else if (obi_atop_e'(sbr_port_req_i.a.a_optional.atop) == ATOPSC) begin + // For a Store-Conditional, only write if the exclusive access was okay. + // Otherwise, perform a dummy read to keep the access order consistent + mgr_port_req_o.a.we = exokay; + end + state_d = state_q; - load_amo = 1'b0; - amo_wb = last_amo_wb & ~rsp_happening; amo_op_d = amo_op_q; + amo_ignore_rsp_d = amo_ignore_rsp_q & ~rsp_happening; + + load_amo = 1'b0; + save_amo_result = 1'b0; unique case (state_q) Idle: begin @@ -371,54 +406,58 @@ module obi_atop_resolver !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, - AMOMINU, AMOMAXU}) begin - mgr_port_req_o.a.we = 1'b0; - end + state_d = WaitAMOLoad; end - end - DoAMO, WriteBackAMO: begin + WaitAMOLoad: begin + // Do not allow any new requests until all outstanding (normal) loads and the AMO load have + // completed sbr_port_rsp_o.gnt = 1'b0; mgr_port_req_o.req = 1'b0; - if (amo_last && rsp_happening) 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 + if (amo_last_outstanding && rsp_happening) begin + save_amo_result = 1'b1; + amo_op_d = obi_atop_e'(ATOPNONE); + state_d = WriteBackAMO; + if (!RegisterAmo) begin + // Forward AMO result + 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.wdata = amo_result; - mgr_port_req_o.a.be = {RiscvWordWidth/8{1'b1}} << + mgr_port_req_o.a.be = {RiscvWordWidth/8{1'b1}} << (amo_operand_addr * RiscvWordWidth/8); + if (mgr_port_rsp_i.gnt && mgr_port_rsp_i.gnt) begin + // Do not forward the write response to the subordinate port + amo_ignore_rsp_d = 1'b1; + state_d = Idle; + end end end end + WriteBackAMO: begin + // The manager port is busy with the write-back; do not grant any new requests + sbr_port_rsp_o.gnt = 1'b0; + // Commit AMO + 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.wdata = amo_result_q; + mgr_port_req_o.a.be = {RiscvWordWidth/8{1'b1}} << + (amo_operand_addr_q * RiscvWordWidth/8); + if (mgr_port_req_o.req && mgr_port_rsp_i.gnt) begin + // Do not forward the write response to the subordinate port + amo_ignore_rsp_d = 1'b1; + state_d = Idle; + end + end default:; endcase 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) - end else begin : gen_amo_slice - assign amo_result_q = '0; - assign amo_operand_addr_q = '0; - end + `FFLNR(amo_result_q, amo_result, save_amo_result, clk_i) + `FFLNR(amo_operand_addr_q, amo_operand_addr, save_amo_result, clk_i) credit_counter #( .NumCredits (NumTxns) @@ -430,29 +469,28 @@ module obi_atop_resolver .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_crit_o(amo_last_outstanding), .credit_full_o() ); always_ff @(posedge clk_i or negedge rst_ni) begin if (!rst_ni) begin - state_q <= Idle; - amo_op_q <= obi_atop_e'('0); - addr_q <= '0; - be_q <= '0; - amo_operand_b_q <= '0; - aid_q <= '0; + state_q <= Idle; + amo_ignore_rsp_q <= 1'b0; + amo_op_q <= obi_atop_e'('0); + addr_q <= '0; + be_q <= '0; + amo_operand_b_q <= '0; + aid_q <= '0; end else begin state_q <= state_d; amo_op_q <= amo_op_d; + amo_ignore_rsp_q <= amo_ignore_rsp_d; if (load_amo) begin - // 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; end end end @@ -463,8 +501,8 @@ module obi_atop_resolver logic [RiscvWordWidth+1:0] adder_sum; logic [RiscvWordWidth:0] adder_operand_a, adder_operand_b; - `FFL(amo_operand_a_q, mgr_port_rsp_i.r.rdata, mgr_port_rsp_i.rvalid, '0, clk_i, rst_ni) - assign amo_operand_a = mgr_port_rsp_i.rvalid ? mgr_port_rsp_i.r.rdata : amo_operand_a_q; + `FFL(amo_operand_a_q, mgr_port_rsp_i.r.rdata, rsp_happening, '0, clk_i, rst_ni) + assign amo_operand_a = rsp_happening ? mgr_port_rsp_i.r.rdata : amo_operand_a_q; assign adder_sum = adder_operand_a + adder_operand_b; /* verilator lint_off WIDTH */ @@ -543,7 +581,7 @@ module obi_atop_resolver_intf /// Enable LR & SC AMOS parameter bit LrScEnable = 1, /// Cut path between request and response at the cost of increased AMO latency - parameter bit RegisterAmo = 1'b0 + parameter bit RegisterAmo = 1'b1 ) ( input logic clk_i, input logic rst_ni, diff --git a/src/test/tb_obi_atop_resolver.sv b/src/test/tb_obi_atop_resolver.sv index 0e4b33d..6e6a59d 100644 --- a/src/test/tb_obi_atop_resolver.sv +++ b/src/test/tb_obi_atop_resolver.sv @@ -185,7 +185,7 @@ module tb_obi_atop_resolver; .SbrPortObiCfg ( MgrMuxedConfig ), .MgrPortObiCfg ( SbrConfig ), .LrScEnable ( 1 ), - .RegisterAmo ( 1'b0 ) + .RegisterAmo ( 1'b1 ) ) i_atop_resolver ( .clk_i ( clk ), .rst_ni ( rst_n ),