From 4e8a3e2e2b85f2213b6a34cc6e95b7f9f0eebfaf Mon Sep 17 00:00:00 2001 From: Maciej Kurc Date: Fri, 24 Nov 2023 14:47:28 +0100 Subject: [PATCH 1/6] Separate wait state for read and write, untangle R and B AXI channels Internal-Tag: [#49716] Signed-off-by: Maciej Kurc --- design/lib/axi4_to_ahb.sv | 45 ++++++++++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 13 deletions(-) diff --git a/design/lib/axi4_to_ahb.sv b/design/lib/axi4_to_ahb.sv index 672f0b18ed9..e4d0bb7a747 100644 --- a/design/lib/axi4_to_ahb.sv +++ b/design/lib/axi4_to_ahb.sv @@ -88,11 +88,21 @@ import el2_pkg::*; localparam ID = 1; localparam PRTY = 1; - typedef enum logic [2:0] {IDLE=3'b000, CMD_RD=3'b001, CMD_WR=3'b010, DATA_RD=3'b011, DATA_WR=3'b100, DONE=3'b101, STREAM_RD=3'b110, STREAM_ERR_RD=3'b111} state_t; + typedef enum logic [3:0] { + IDLE = 4'b0000, + CMD_RD = 4'b0001, + CMD_WR = 4'b1001, + DATA_RD = 4'b0010, + DATA_WR = 4'b1010, + DONE_RD = 4'b0011, + DONE_WR = 4'b1011, + STREAM_RD = 4'b0101, + STREAM_ERR_RD = 4'b0110 + } state_t; + state_t buf_state, buf_nxtstate; logic slave_valid; - logic slave_ready; logic [TAG-1:0] slave_tag; logic [63:0] slave_rdata; logic [3:0] slave_opc; @@ -245,15 +255,14 @@ import el2_pkg::*; assign master_wdata[63:0] = wrbuf_data[63:0]; // AXI response channel signals - assign axi_bvalid = slave_valid & slave_ready & slave_opc[3]; + assign axi_bvalid = slave_valid & axi_bready & slave_opc[3]; assign axi_bresp[1:0] = slave_opc[0] ? 2'b10 : (slave_opc[1] ? 2'b11 : 2'b0); assign axi_bid[TAG-1:0] = slave_tag[TAG-1:0]; - assign axi_rvalid = slave_valid & slave_ready & (slave_opc[3:2] == 2'b0); + assign axi_rvalid = slave_valid & axi_rready & (slave_opc[3:2] == 2'b0); assign axi_rresp[1:0] = slave_opc[0] ? 2'b10 : (slave_opc[1] ? 2'b11 : 2'b0); assign axi_rid[TAG-1:0] = slave_tag[TAG-1:0]; assign axi_rdata[63:0] = slave_rdata[63:0]; - assign slave_ready = axi_bready & axi_rready; // FIFO state machine always_comb begin @@ -324,7 +333,7 @@ import el2_pkg::*; ahb_htrans[1:0] = 2'b10 & {2{~buf_state_en}}; end DATA_RD: begin - buf_nxtstate = DONE; + buf_nxtstate = DONE_RD; buf_state_en = (ahb_hready_q | ahb_hresp_q); buf_data_wr_en = buf_state_en; slvbuf_error_in= ahb_hresp_q; @@ -345,8 +354,8 @@ import el2_pkg::*; end DATA_WR: begin buf_state_en = (cmd_doneQ & ahb_hready_q) | ahb_hresp_q; - master_ready = buf_state_en & ~ahb_hresp_q & slave_ready; // Ready to accept new command if current command done and no error - buf_nxtstate = (ahb_hresp_q | ~slave_ready) ? DONE : + master_ready = buf_state_en & ~ahb_hresp_q & axi_bready; // Ready to accept new command if current command done and no error + buf_nxtstate = (ahb_hresp_q | ~axi_bready) ? DONE_WR : ((master_valid & master_ready) ? ((master_opc[2:1] == 2'b01) ? CMD_WR : CMD_RD) : IDLE); slvbuf_error_in = ahb_hresp_q; slvbuf_error_en = buf_state_en; @@ -359,19 +368,29 @@ import el2_pkg::*; ((buf_cmd_byte_ptrQ == 3'b111) | (buf_byteen[get_nxtbyte_ptr(buf_cmd_byte_ptrQ[2:0],buf_byteen[7:0],1'b1)] == 1'b0)))); bypass_en = buf_state_en & buf_write_in & (buf_nxtstate == CMD_WR); // Only bypass for writes for the time being ahb_htrans[1:0] = {2{(~(cmd_done | cmd_doneQ) | bypass_en)}} & 2'b10; - slave_valid_pre = buf_state_en & (buf_nxtstate != DONE); + slave_valid_pre = buf_state_en & (buf_nxtstate != DONE_WR); trxn_done = ahb_hready_q & ahb_hwrite_q & (ahb_htrans_q[1:0] != 2'b0); buf_cmd_byte_ptr_en = trxn_done | bypass_en; buf_cmd_byte_ptr = bypass_en ? get_nxtbyte_ptr(3'b0,buf_byteen_in[7:0],1'b0) : trxn_done ? get_nxtbyte_ptr(buf_cmd_byte_ptrQ[2:0],buf_byteen[7:0],1'b1) : buf_cmd_byte_ptrQ; - end - DONE: begin + end + DONE_WR: begin + buf_nxtstate = IDLE; + buf_state_en = axi_bvalid & axi_bready; + slvbuf_error_en = 1'b1; + slave_valid_pre = 1'b1; + end + DONE_RD: begin buf_nxtstate = IDLE; - buf_state_en = slave_ready; + buf_state_en = axi_rvalid & axi_rready & axi_rlast; slvbuf_error_en = 1'b1; slave_valid_pre = 1'b1; end + default: begin + buf_nxtstate = IDLE; + buf_state_en = 1'b1; + end endcase end @@ -403,7 +422,7 @@ import el2_pkg::*; assign slave_valid = slave_valid_pre;// & (~slvbuf_posted_write | slvbuf_error); assign slave_opc[3:2] = slvbuf_write ? 2'b11 : 2'b00; assign slave_opc[1:0] = {2{slvbuf_error}} & 2'b10; - assign slave_rdata[63:0] = slvbuf_error ? {2{last_bus_addr[31:0]}} : ((buf_state == DONE) ? buf_data[63:0] : ahb_hrdata_q[63:0]); + assign slave_rdata[63:0] = slvbuf_error ? {2{last_bus_addr[31:0]}} : ((buf_state == DONE_RD) ? buf_data[63:0] : ahb_hrdata_q[63:0]); assign slave_tag[TAG-1:0] = slvbuf_tag[TAG-1:0]; assign last_addr_en = (ahb_htrans[1:0] != 2'b0) & ahb_hready & ahb_hwrite ; From 52c2f7766e3246bdd6d019bfe531cb3719dd0fea Mon Sep 17 00:00:00 2001 From: Maciej Kurc Date: Fri, 24 Nov 2023 15:26:18 +0100 Subject: [PATCH 2/6] Remove dependency of AXI valid on AXI ready Internal-Tag: [#49716] Signed-off-by: Maciej Kurc --- design/lib/axi4_to_ahb.sv | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/design/lib/axi4_to_ahb.sv b/design/lib/axi4_to_ahb.sv index e4d0bb7a747..12879f4f882 100644 --- a/design/lib/axi4_to_ahb.sv +++ b/design/lib/axi4_to_ahb.sv @@ -255,11 +255,11 @@ import el2_pkg::*; assign master_wdata[63:0] = wrbuf_data[63:0]; // AXI response channel signals - assign axi_bvalid = slave_valid & axi_bready & slave_opc[3]; + assign axi_bvalid = slave_valid & slave_opc[3]; assign axi_bresp[1:0] = slave_opc[0] ? 2'b10 : (slave_opc[1] ? 2'b11 : 2'b0); assign axi_bid[TAG-1:0] = slave_tag[TAG-1:0]; - assign axi_rvalid = slave_valid & axi_rready & (slave_opc[3:2] == 2'b0); + assign axi_rvalid = slave_valid & (slave_opc[3:2] == 2'b0); assign axi_rresp[1:0] = slave_opc[0] ? 2'b10 : (slave_opc[1] ? 2'b11 : 2'b0); assign axi_rid[TAG-1:0] = slave_tag[TAG-1:0]; assign axi_rdata[63:0] = slave_rdata[63:0]; From 71a07ed617312ac1f15d6916a5395985a3f58522 Mon Sep 17 00:00:00 2001 From: Maciej Kurc Date: Fri, 24 Nov 2023 15:53:02 +0100 Subject: [PATCH 3/6] Update axi4_to_ahb module tests Internal-Tag: [#49716] Signed-off-by: Maciej Kurc --- .../block/lib_axi4_to_ahb/axi_r_bfm.py | 7 ++-- .../block/lib_axi4_to_ahb/coordinator_seq.py | 6 ++- .../block/lib_axi4_to_ahb/test_axi.py | 39 +------------------ .../lib_axi4_to_ahb/test_axi_read_channel.py | 5 +-- .../lib_axi4_to_ahb/test_axi_write_channel.py | 5 +-- 5 files changed, 12 insertions(+), 50 deletions(-) diff --git a/verification/block/lib_axi4_to_ahb/axi_r_bfm.py b/verification/block/lib_axi4_to_ahb/axi_r_bfm.py index 08cb20945ed..9f3db17313c 100644 --- a/verification/block/lib_axi4_to_ahb/axi_r_bfm.py +++ b/verification/block/lib_axi4_to_ahb/axi_r_bfm.py @@ -109,9 +109,10 @@ async def rsp_monitor_q_bfm(self): await RisingEdge(self.rst_n) await RisingEdge(self.clk) if get_int(self.dut.axi_rvalid): - sigs = get_signals(AXI_R_CHAN_RSP_SIGNALS, self.dut) - values = tuple(sig.value for sig in sigs) - await self.rsp_monitor_q.put(values) + if get_int(self.dut.axi_rready): + sigs = get_signals(AXI_R_CHAN_RSP_SIGNALS, self.dut) + values = tuple(sig.value for sig in sigs) + await self.rsp_monitor_q.put(values) def start_bfm(self): cocotb.start_soon(self.drive()) diff --git a/verification/block/lib_axi4_to_ahb/coordinator_seq.py b/verification/block/lib_axi4_to_ahb/coordinator_seq.py index e0a0cf9a40b..7300f3a3be7 100644 --- a/verification/block/lib_axi4_to_ahb/coordinator_seq.py +++ b/verification/block/lib_axi4_to_ahb/coordinator_seq.py @@ -6,7 +6,7 @@ import cocotb from ahb_lite_pkg import AHB_LITE_NOTIFICATION from ahb_lite_seq import AHBLiteAcceptReadSeq, AHBLiteAcceptWriteSeq -from axi_r_seq import AXIReadTransactionRequestSeq +from axi_r_seq import AXIReadTransactionRequestSeq, AXIReadTransactionResponseSeq from axi_w_seq import ( AXIWriteDataSeq, AXIWriteResponseSeq, @@ -37,6 +37,7 @@ async def axi_write(self, axi_seqr, ahb_seqr): async def axi_read(self, axi_seqr, ahb_seqr): axi_trq_seq = AXIReadTransactionRequestSeq() + axi_rresp_seq = AXIReadTransactionResponseSeq() # Read Request await axi_trq_seq.start(axi_seqr) @@ -46,6 +47,9 @@ async def axi_read(self, axi_seqr, ahb_seqr): await self.ahb_response_handler(ahb_seqr=ahb_seqr, is_read=True) await self.delay(5) + # Read Response + await axi_rresp_seq.start(axi_seqr) + async def delay(self, i): for _ in range(i): await RisingEdge(cocotb.top.clk) diff --git a/verification/block/lib_axi4_to_ahb/test_axi.py b/verification/block/lib_axi4_to_ahb/test_axi.py index 8a020e3b53a..c1f63293c1c 100644 --- a/verification/block/lib_axi4_to_ahb/test_axi.py +++ b/verification/block/lib_axi4_to_ahb/test_axi.py @@ -6,45 +6,8 @@ from coordinator_seq import TestBothChannelsSeq from testbench import BaseTest -# FIXME : This test is expected to fail. -# Reason : Handshake sequence is non-compliant with specification -# Faulty code : axi4_to_ahb.sv#L248-256 -# -# Issue #1 BVALID/BREADY Handshake -# Handshake is meant to occur on the Write Response Channel in order: -# * subordinate asserts BVALID -# * manager responds with BREADY -# Quote: "The Subordinate must not wait for the Manager to assert BREADY -# before asserting BVALID" -# Source: AMBA AXI Protocol Specification A3.5.1 Write transaction dependencies -# -# In RTL: -# -# assign axi_bvalid = slave_valid & slave_ready & slave_opc[3]; -# assign slave_ready = axi_bready & axi_rready; -# -# BVALID is calculated from BREADY and RREADY, which is wrong for 2 reasons: -# * BVALID should not depend on RREADY -# * BVALID should be asserted before BREADY. BREADY should depend on BVALID. -# -# Issue #2 RVALID/RREADY Handshake -# Handshake is meant to occur on the Read Response Channel in order: -# * subordinate asserts RVALID -# * manager responds with RREADY -# Quote: "The Subordinate must not wait for the Manager to assert RREADY -# before asserting RVALID" -# Source: AMBA AXI Protocol Specification A3.5.2 Read transaction dependencies -# -# In RTL: -# assign axi_rvalid = slave_valid & slave_ready & (slave_opc[3:2] == 2'b0); -# assign slave_ready = axi_bready & axi_rready; -# -# RVALID is calculated from BREADY and RREADY, which is wrong for 2 reasons: -# * RVALID should not depend on BREADY -# * RVALID should be asserted before RREADY. RREADY should depend on RVALID. - -@pyuvm.test(expect_error=(TimeoutError, QueueFull)) +@pyuvm.test() class TestAXI(BaseTest): def end_of_elaboration_phase(self): self.seq = TestBothChannelsSeq.create("stimulus") diff --git a/verification/block/lib_axi4_to_ahb/test_axi_read_channel.py b/verification/block/lib_axi4_to_ahb/test_axi_read_channel.py index 1caa2cbc010..f13aaf526bb 100644 --- a/verification/block/lib_axi4_to_ahb/test_axi_read_channel.py +++ b/verification/block/lib_axi4_to_ahb/test_axi_read_channel.py @@ -6,11 +6,8 @@ from coordinator_seq import TestReadChannelSeq from testbench import BaseTest -# FIXME : This test is expected to fail. -# See description in `test_axi.py` - -@pyuvm.test(expect_error=QueueFull) +@pyuvm.test() class TestAXIReadChannel(BaseTest): def end_of_elaboration_phase(self): self.seq = TestReadChannelSeq.create("stimulus") diff --git a/verification/block/lib_axi4_to_ahb/test_axi_write_channel.py b/verification/block/lib_axi4_to_ahb/test_axi_write_channel.py index 598f7afe9b5..9519bae3b42 100644 --- a/verification/block/lib_axi4_to_ahb/test_axi_write_channel.py +++ b/verification/block/lib_axi4_to_ahb/test_axi_write_channel.py @@ -5,11 +5,8 @@ from coordinator_seq import TestWriteChannelSeq from testbench import BaseTest -# FIXME : This test is expected to fail. -# See description in `test_axi.py` - -@pyuvm.test(expect_error=TimeoutError) +@pyuvm.test() class TestAXIWriteChannel(BaseTest): def end_of_elaboration_phase(self): self.seq = TestWriteChannelSeq.create("stimulus") From 6dbaf829fb0682836bef1bca62b1f7101e07ae93 Mon Sep 17 00:00:00 2001 From: Maciej Kurc Date: Fri, 24 Nov 2023 15:54:01 +0100 Subject: [PATCH 4/6] Fix testbench to work with AHB, VeeR config options as Makefile parameter. Internal-Tag: [#49716] Signed-off-by: Maciej Kurc --- testbench/tb_top.sv | 7 +++++++ tools/Makefile | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/testbench/tb_top.sv b/testbench/tb_top.sv index 94c5257ba6b..3a7834173b3 100644 --- a/testbench/tb_top.sv +++ b/testbench/tb_top.sv @@ -328,8 +328,15 @@ module tb_top ( `define DEC rvtop.veer.dec +`ifdef RV_BUILD_AXI4 assign mailbox_write = lmem.awvalid && lmem.awaddr == mem_mailbox && rst_l; assign mailbox_data = lmem.wdata; +`endif +`ifdef RV_BUILD_AHB_LITE + assign mailbox_write = lmem.write && lmem.laddr == mem_mailbox && rst_l; + assign mailbox_data = lmem.HWDATA; +`endif + assign mailbox_data_val = mailbox_data[7:0] > 8'h5 && mailbox_data[7:0] < 8'h7f; parameter MAX_CYCLES = 2_000_000; diff --git a/tools/Makefile b/tools/Makefile index 371653c527a..218d9211e25 100755 --- a/tools/Makefile +++ b/tools/Makefile @@ -14,7 +14,7 @@ # limitations under the License. # -CONF_PARAMS = -set build_axi4 +CONF_PARAMS ?= -set build_axi4 TEST_CFLAGS = -g -O3 -funroll-all-loops ABI = -mabi=ilp32 -march=rv32imac From 9d5bcadb4faf7daf77012123d8e04f40c35bc663 Mon Sep 17 00:00:00 2001 From: Maciej Kurc Date: Mon, 27 Nov 2023 10:33:29 +0100 Subject: [PATCH 5/6] Add running regression tests for VeeR with AHB to the CI Internal-Tag: [#49716] Signed-off-by: Maciej Kurc --- .github/scripts/run_regression_test.sh | 25 +++++++++++++++++++------ .github/workflows/test-regression.yml | 5 +++-- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/.github/scripts/run_regression_test.sh b/.github/scripts/run_regression_test.sh index 4598af7b1b5..796704dbbc2 100755 --- a/.github/scripts/run_regression_test.sh +++ b/.github/scripts/run_regression_test.sh @@ -7,17 +7,29 @@ run_regression_test(){ # Run a regression test with coverage collection enabled # Args: # RESULTS_DIR - + # BUS - # NAME - # COVERAGE - - check_args_count $# 3 + check_args_count $# 4 RESULTS_DIR=$1 - NAME=$2 - COVERAGE=$3 + BUS=$2 + NAME=$3 + COVERAGE=$4 echo -e "${COLOR_WHITE}========== running test '${NAME}' =========${COLOR_CLEAR}" echo -e "${COLOR_WHITE} RESULTS_DIR = ${RESULTS_DIR}${COLOR_CLEAR}" + echo -e "${COLOR_WHITE} SYSTEM BUS = ${BUS}${COLOR_CLEAR}" echo -e "${COLOR_WHITE} NAME = ${NAME}${COLOR_CLEAR}" echo -e "${COLOR_WHITE} COVERAGE = ${COVERAGE}${COLOR_CLEAR}" + if [[ "${BUS}" == "axi" ]]; then + PARAMS="-set build_axi4" + elif [[ "${BUS}" == "ahb" ]]; then + PARAMS="-set build_ahb_lite" + else + echo -e "${COLOR_RED}Unknown system bus type '${BUS}'${COLOR_CLEAR}" + exit 1 + fi + mkdir -p ${RESULTS_DIR} LOG="${RESULTS_DIR}/test_${NAME}_${COVERAGE}.log" touch ${LOG} @@ -25,7 +37,7 @@ run_regression_test(){ # Run the test mkdir -p ${DIR} - make -j`nproc` -C ${DIR} -f $RV_ROOT/tools/Makefile verilator TEST=${NAME} COVERAGE=${COVERAGE} 2>&1 | tee ${LOG} + make -j`nproc` -C ${DIR} -f $RV_ROOT/tools/Makefile verilator CONF_PARAMS="${PARAMS}" TEST=${NAME} COVERAGE=${COVERAGE} 2>&1 | tee ${LOG} if [ ! -f "${DIR}/coverage.dat" ]; then echo -e "${COLOR_WHITE}Test '${NAME}' ${COLOR_RED}FAILED${COLOR_CLEAR}" exit 1 @@ -38,11 +50,12 @@ run_regression_test(){ # Example usage # RESULTS_DIR=results +# BUS=axi # NAME=hello_world # COVERAGE=branch -# run_regression_test.sh $RESULTS_DIR $NAME $COVERAGE +# run_regression_test.sh $RESULTS_DIR $BUS $NAME $COVERAGE -check_args_count $# 3 +check_args_count $# 4 run_regression_test "$@" diff --git a/.github/workflows/test-regression.yml b/.github/workflows/test-regression.yml index 6342b3600a2..e84d0352976 100644 --- a/.github/workflows/test-regression.yml +++ b/.github/workflows/test-regression.yml @@ -10,6 +10,7 @@ jobs: runs-on: ubuntu-latest strategy: matrix: + bus: ["axi", "ahb"] test: ["hello_world", "hello_world_dccm", "hello_world_iccm", "cmark", "cmark_dccm", "cmark_iccm", "dhry", "pmp"] coverage: ["branch", "toggle"] #TODO: add functional coverage env: @@ -82,7 +83,7 @@ jobs: run: | export PATH=/opt/verilator/bin:$PATH export RV_ROOT=`pwd` - .github/scripts/run_regression_test.sh $TEST_PATH ${{ matrix.test}} ${{ matrix.coverage }} + .github/scripts/run_regression_test.sh $TEST_PATH ${{ matrix.bus }} ${{ matrix.test}} ${{ matrix.coverage }} - name: Prepare coverage data run: | @@ -90,7 +91,7 @@ jobs: echo "convert_coverage_data.sh exited with RET_CODE = "$? mkdir -p results mv ${TEST_PATH}/coverage.info \ - results/coverage_${{ matrix.test }}_${{ matrix.coverage }}.info + results/coverage_${{ matrix.bus }}_${{ matrix.test }}_${{ matrix.coverage }}.info - name: Pack artifacts if: always() From 7b1e946c5fa9f8fa91d7ea9b25b0287820ffd35e Mon Sep 17 00:00:00 2001 From: Maciej Kurc Date: Tue, 28 Nov 2023 09:28:16 +0100 Subject: [PATCH 6/6] Remove unnecessary AND operation. Internal-Tag: [#49716] Signed-off-by: Maciej Kurc --- design/lib/axi4_to_ahb.sv | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/design/lib/axi4_to_ahb.sv b/design/lib/axi4_to_ahb.sv index 12879f4f882..af9fcf2f2d8 100644 --- a/design/lib/axi4_to_ahb.sv +++ b/design/lib/axi4_to_ahb.sv @@ -383,7 +383,7 @@ import el2_pkg::*; end DONE_RD: begin buf_nxtstate = IDLE; - buf_state_en = axi_rvalid & axi_rready & axi_rlast; + buf_state_en = axi_rvalid & axi_rready; // axi_rlast == 1 slvbuf_error_en = 1'b1; slave_valid_pre = 1'b1; end