From c308e8ac84a239761ce999b5d3882094a4ff73bb Mon Sep 17 00:00:00 2001 From: Rupert Swarbrick Date: Fri, 17 Jan 2025 12:47:36 +0000 Subject: [PATCH] [tlul,rtl] Correct the calculation of data_intg in tlul_adapter_sram This is the value that gets supplied as a user field that provides data integrity. If we are responding with an error response, we need to make sure we send integrity bits that correspond. The logic that was previously here detected this condition with (vld_rd_rsp && reqfifo_rdata.error) but that is wrong because it shouldn't depend on vld_rd_rsp. Imagine we send a TL write with a TL error. When we read the response, the d_error flag will be high (because u_reqfifo contains the faulty TL write) and d_data will be error_blanking_data. But the integrity bits will be SecdedInv3932ZeroEcc because vld_rd_rsp is false (we haven't seen a TL read at all!) This is also possible to trigger by using only reads. Suppose we send an TL read with a TL error and then, a few cycles later, read the TL response. When we read the response, the d_error flag will again be high (because u_reqfifo contains the faulty TL read). Again, d_data will (correctly) be error_blanking_data. Again, we should be using error_blanking_integ for error bits but we actually use SecdedInv3932ZeroEcc. Dropping the vld_rd_rsp term will fix the behaviour in both cases. So it remains in sync with the RTL, we also drop a conditional coverage exclusion for rom_ctrl. Tracking down how it was actually possible to see this happen led us to the design change. Signed-off-by: Rupert Swarbrick --- hw/ip/rom_ctrl/dv/cov/rom_ctrl_cov_excl.el | 9 --------- hw/ip/tlul/rtl/tlul_adapter_sram.sv | 6 +++--- 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/hw/ip/rom_ctrl/dv/cov/rom_ctrl_cov_excl.el b/hw/ip/rom_ctrl/dv/cov/rom_ctrl_cov_excl.el index 102f6ab73bf1e..5a2af3e23cb16 100644 --- a/hw/ip/rom_ctrl/dv/cov/rom_ctrl_cov_excl.el +++ b/hw/ip/rom_ctrl/dv/cov/rom_ctrl_cov_excl.el @@ -98,15 +98,6 @@ Condition 21 "3623514242" "(d_valid & rspfifo_rvalid & (reqfifo_rdata.op == OpRe // popped again before anything appears in the response fifo. Condition 24 "1059982851" "(vld_rd_rsp & ((~d_error))) 1 -1" (2 "10") -// The reason for condition 24 explains why we can't get (vld_rd_rsp && reqfifo_rdata.error) = 1 as -// if vld_rd_rsp = 1, this means that rspfifo_rvalid = 1. But when we have reqfifo_rdata.error, the -// request from the req_fifo gets popped again before becoming visible to response fifo. This also -// explains why we can't expect 1 and 11 for condition 25 and 26 respectively. -Condition 25 "2807788926" "((vld_rd_rsp && reqfifo_rdata.error) ? error_blanking_integ : - (vld_rd_rsp ? rspfifo_rdata.data_intg : - prim_secded_pkg::SecdedInv3932ZeroEcc)) 1 -1" (2 "1") -Condition 26 "561780173" "(vld_rd_rsp && reqfifo_rdata.error) 1 -1" (3 "11") - // We cannot see d_valid = 0 and d_error = 1. For d_error to be true, we need reqfifo_rvalid to be // true. But then the only way for d_valid to be false is if rspfifo_rvalid is false (and // reqfifo_rdata.op is OpRead). diff --git a/hw/ip/tlul/rtl/tlul_adapter_sram.sv b/hw/ip/tlul/rtl/tlul_adapter_sram.sv index 8e0100881a6de..696535c8e508f 100644 --- a/hw/ip/tlul/rtl/tlul_adapter_sram.sv +++ b/hw/ip/tlul/rtl/tlul_adapter_sram.sv @@ -345,9 +345,9 @@ module tlul_adapter_sram // If this a write response with data fields set to 0, we have to set all ECC bits correctly // since we are using an inverted Hsiao code. logic [DataIntgWidth-1:0] data_intg; - assign data_intg = (vld_rd_rsp && reqfifo_rdata.error) ? error_blanking_integ : // TL-UL error - (vld_rd_rsp) ? rspfifo_rdata.data_intg : // valid read - prim_secded_pkg::SecdedInv3932ZeroEcc; // valid write + assign data_intg = (reqfifo_rdata.error) ? error_blanking_integ : // TL-UL error + (vld_rd_rsp) ? rspfifo_rdata.data_intg : // valid read + prim_secded_pkg::SecdedInv3932ZeroEcc; // valid write // When an error is seen on an incoming transaction it gets an immediate response without // performing an SRAM request. It may be the transaction receives a ready the first cycle it is