From f3dc603722b26c1a351fd556abed183bdf37fb8b Mon Sep 17 00:00:00 2001 From: Rupert Swarbrick Date: Thu, 30 Jan 2025 16:48:18 +0000 Subject: [PATCH 1/2] [prim,rtl] Rewrite a case as if/else to avoid unreachable case The behaviour should be unchanged (StIdle means idle_q=1; StWait means idle_q=0). Signed-off-by: Rupert Swarbrick --- hw/ip/prim/rtl/prim_reg_cdc_arb.sv | 60 ++++++++++++------------------ 1 file changed, 24 insertions(+), 36 deletions(-) diff --git a/hw/ip/prim/rtl/prim_reg_cdc_arb.sv b/hw/ip/prim/rtl/prim_reg_cdc_arb.sv index 30ff91e79bb63..7832a83c3b080 100644 --- a/hw/ip/prim/rtl/prim_reg_cdc_arb.sv +++ b/hw/ip/prim/rtl/prim_reg_cdc_arb.sv @@ -87,11 +87,6 @@ module prim_reg_cdc_arb #( SelHwReq } req_sel_e; - typedef enum logic { - StIdle, - StWait - } state_e; - // Only honor the incoming destination update request if the incoming // value is actually different from what is already completed in the @@ -106,12 +101,12 @@ module prim_reg_cdc_arb #( logic dst_update_ack; req_sel_e id_q; - state_e state_q, state_d; + logic idle_q, idle_d; always_ff @(posedge clk_dst_i or negedge rst_dst_ni) begin if (!rst_dst_ni) begin - state_q <= StIdle; + idle_q <= 1'b1; end else begin - state_q <= state_d; + idle_q <= idle_d; end end @@ -194,7 +189,7 @@ module prim_reg_cdc_arb #( logic dst_hold_req; always_comb begin - state_d = state_q; + idle_d = idle_q; dst_hold_req = '0; // depending on when the request is received, we @@ -204,35 +199,28 @@ module prim_reg_cdc_arb #( busy = 1'b1; - unique case (state_q) - StIdle: begin - busy = '0; - if (dst_req) begin - // there's a software issued request for change - state_d = StWait; - dst_lat_d = 1'b1; - end else if (dst_update) begin - state_d = StWait; - dst_lat_d = 1'b1; - end else if (dst_qs_o != dst_qs_i) begin - // there's a direct destination update - // that was blocked by an ongoing transaction - state_d = StWait; - dst_lat_q = 1'b1; - end - end - - StWait: begin - dst_hold_req = 1'b1; - if (dst_update_ack) begin - state_d = StIdle; - end + if (idle_q) begin + busy = 1'b0; + + if (dst_req) begin + // there's a software issued request for change + idle_d = 1'b0; + dst_lat_d = 1'b1; + end else if (dst_update) begin + idle_d = 1'b0; + dst_lat_d = 1'b1; + end else if (dst_qs_o != dst_qs_i) begin + // there's a direct destination update + // that was blocked by an ongoing transaction + idle_d = 1'b0; + dst_lat_q = 1'b1; end - - default: begin - state_d = StIdle; + end else begin + dst_hold_req = 1'b1; + if (dst_update_ack) begin + idle_d = 1'b1; end - endcase // unique case (state_q) + end end // always_comb assign dst_update_req = dst_hold_req | dst_lat_d | dst_lat_q; From f47aaa85f851b00f5b37d3078e0cc493a24dbbce Mon Sep 17 00:00:00 2001 From: Rupert Swarbrick Date: Wed, 12 Feb 2025 13:24:43 +0000 Subject: [PATCH 2/2] [prim,rtl] Get rid of a trivial combinatorial variable Busy is true exactly if the next state will not be idle. Use that naming instead. Signed-off-by: Rupert Swarbrick --- hw/ip/prim/rtl/prim_reg_cdc_arb.sv | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/hw/ip/prim/rtl/prim_reg_cdc_arb.sv b/hw/ip/prim/rtl/prim_reg_cdc_arb.sv index 7832a83c3b080..964ddae5add90 100644 --- a/hw/ip/prim/rtl/prim_reg_cdc_arb.sv +++ b/hw/ip/prim/rtl/prim_reg_cdc_arb.sv @@ -110,7 +110,6 @@ module prim_reg_cdc_arb #( end end - logic busy; logic dst_req_q, dst_req; always_ff @(posedge clk_dst_i or negedge rst_dst_ni) begin if (!rst_dst_ni) begin @@ -121,7 +120,7 @@ module prim_reg_cdc_arb #( // dst_lat_d is safe to used here because dst_req_q, if set, // always has priority over other hardware based events. dst_req_q <= '0; - end else if (dst_req_i && busy) begin + end else if (dst_req_i && !idle_q) begin // if destination request arrives when a handshake event // is already ongoing, hold on to request and send later dst_req_q <= 1'b1; @@ -129,7 +128,7 @@ module prim_reg_cdc_arb #( end // dst_req_q will be 0 when dst_req_i is set, this assertion checks the conditional branch - // (dst_req_i && !dst_req_q && busy) can be simplified to avoid conditional coverage + // (dst_req_i && !dst_req_q && !idle_q) can be simplified to avoid conditional coverage // holes `ASSERT(Not_Dst_req_q_while_dst_req_i_A, dst_req_i |-> !dst_req_q, clk_dst_i, !rst_dst_ni) @@ -172,7 +171,7 @@ module prim_reg_cdc_arb #( // if a destination update is received when the system is idle and there is no // software side request, hw update must be selected. - `ASSERT(DstUpdateReqCheck_A, ##1 dst_update & !dst_req & !busy |=> id_q == SelHwReq, + `ASSERT(DstUpdateReqCheck_A, ##1 dst_update & !dst_req & idle_q |=> id_q == SelHwReq, clk_dst_i, !rst_dst_ni) // if hw select was chosen, then it must be the case there was a destination update @@ -185,7 +184,7 @@ module prim_reg_cdc_arb #( // send out prim_subreg request only when proceeding // with software request - assign dst_req_o = ~busy & dst_req; + assign dst_req_o = idle_q & dst_req; logic dst_hold_req; always_comb begin @@ -197,11 +196,7 @@ module prim_reg_cdc_arb #( dst_lat_q = '0; dst_lat_d = '0; - busy = 1'b1; - if (idle_q) begin - busy = 1'b0; - if (dst_req) begin // there's a software issued request for change idle_d = 1'b0; @@ -258,7 +253,7 @@ module prim_reg_cdc_arb #( async_flag <= '0; end else if (src_update_o) begin async_flag <= '0; - end else if (dst_update && !dst_req_o && !busy) begin + end else if (dst_update && !dst_req_o && idle_q) begin async_flag <= 1'b1; end end