From f444d492bcfab4a74310232bf5d3bea2d21d4045 Mon Sep 17 00:00:00 2001 From: Robert Szczepanski Date: Wed, 13 Nov 2024 11:59:29 +0100 Subject: [PATCH] Improve target reset pattern detection Signed-off-by: Robert Szczepanski --- src/ctrl/bus_monitor.sv | 28 +++---- src/ctrl/target_reset_detector.sv | 83 +++++++------------ third_party/cocotbext-i3c | 2 +- .../ctrl_bus_monitor/test_bus_monitor.py | 9 +- violations.waiver | 1 + 5 files changed, 51 insertions(+), 72 deletions(-) diff --git a/src/ctrl/bus_monitor.sv b/src/ctrl/bus_monitor.sv index 1bacd64..a25e778 100644 --- a/src/ctrl/bus_monitor.sv +++ b/src/ctrl/bus_monitor.sv @@ -29,9 +29,9 @@ module bus_monitor output logic start_detect_o, // Module detected START or REPEATED START condition output logic stop_detect_o, // Module detected STOP condition - input logic is_in_hdr_mode_i, // Module is in HDR mode + input logic is_in_hdr_mode_i, // Module is in HDR mode output logic hdr_exit_detect_o, // Detected HDR exit condition (see: 5.2.1.1.1 of the base spec) - output logic target_reset_detect_o // Deteced Target Reset condtition + output logic target_reset_detect_o // Detected Target Reset condtition ); logic enable, enable_q; @@ -229,17 +229,17 @@ module bus_monitor assign stop_detect_o = stop_det; assign hdr_exit_detect_o = hdr_exit_det; - target_reset_detector target_reset_detector( - .clk_i, - .rst_ni, - .enable_i, - .scl_high(scl_stable_high), - .scl_low(scl_stable_low), - .scl_negedge, - .sda_posedge, - .sda_negedge, - .start_detected_i(start_det), - .stop_detected_i(stop_det), - .target_reset_detect_o + target_reset_detector target_reset_detector ( + .clk_i, + .rst_ni, + .enable_i, + .scl_high(scl_stable_high), + .scl_low(scl_stable_low), + .scl_negedge, + .sda_posedge, + .sda_negedge, + .start_detected_i(start_det), + .stop_detected_i(stop_det), + .target_reset_detect_o ); endmodule diff --git a/src/ctrl/target_reset_detector.sv b/src/ctrl/target_reset_detector.sv index 362c62e..aee9c6f 100644 --- a/src/ctrl/target_reset_detector.sv +++ b/src/ctrl/target_reset_detector.sv @@ -11,7 +11,7 @@ module target_reset_detector input logic clk_i, input logic rst_ni, - input logic enable_i, // Enable + input logic enable_i, input logic scl_low, input logic scl_high, @@ -19,66 +19,42 @@ module target_reset_detector input logic sda_posedge, input logic sda_negedge, - input start_detected_i, stop_detected_i, + input start_detected_i, + stop_detected_i, output logic target_reset_detect_o ); - logic count_sda_transition; + logic count_sda_transition_en; - logic [3:0] transition_count_q; - logic [3:0] transition_count_d; - logic [1:0] suspicious_transition_count_q; - logic [1:0] suspicious_transition_count_d; - logic invalidate_sr; + logic [3:0] sda_transition_count_q; + logic [3:0] sda_transition_count_d; target_reset_detector_state_e state_q, state_d; always_comb begin - if (scl_high) - transition_count_d = 4'h0; + if (scl_high) sda_transition_count_d = 4'h0; else - transition_count_d = - count_sda_transition ? transition_count_q + 4'h1 : transition_count_q; + sda_transition_count_d = count_sda_transition_en ? + sda_transition_count_q + 4'h1 : + sda_transition_count_q; - if ((state_q inside {AwaitP, AwaitSr}) & (sda_posedge | sda_negedge)) begin - suspicious_transition_count_d = suspicious_transition_count_q + 2'h1; - end else if (state_q == AwaitPattern) begin - suspicious_transition_count_d = 2'h0; - end - - count_sda_transition = 0; - if ((state_q == AwaitPattern) & (transition_count_q < 4'he)) - count_sda_transition = (sda_posedge & (transition_count_q != 0)) | sda_negedge; - - invalidate_sr = suspicious_transition_count_d > 1; + if ((state_q == AwaitPattern) & (sda_transition_count_q < 4'he)) + count_sda_transition_en = (sda_posedge & (sda_transition_count_q != 0)) | sda_negedge; + else count_sda_transition_en = 0; end - always_ff @(posedge clk_i or negedge rst_ni) begin : target_reset_transition_counter - if (!rst_ni) begin - transition_count_q <= 4'h0; - end else if (clk_i) begin - transition_count_q <= transition_count_d; - end - end : target_reset_transition_counter - - // If suspicious_transition_count == 1, it's either start or stop signal, - // but if it's more then it means something weird has happened or start/stop timing - // requirements were not met - always_ff @(posedge clk_i or negedge rst_ni) begin : suspicious_transition_counter + always_ff @(posedge clk_i or negedge rst_ni) begin : target_reset_sda_transition_counter if (!rst_ni) begin - suspicious_transition_count_q <= 2'h0; - end else if (clk_i) begin - if (state_d == state_q) - suspicious_transition_count_q <= suspicious_transition_count_d; - else - suspicious_transition_count_q <= 2'h0; // Reset on state transition + sda_transition_count_q <= 4'h0; + end else begin + sda_transition_count_q <= sda_transition_count_d; end - end: suspicious_transition_counter + end : target_reset_sda_transition_counter always_ff @(posedge clk_i or negedge rst_ni) begin : target_reset_detection_state if (!rst_ni) begin state_q <= AwaitPattern; - end else if (clk_i) begin + end else begin state_q <= state_d; end end : target_reset_detection_state @@ -86,23 +62,26 @@ module target_reset_detector always_comb begin : target_reset_detection_fsm case (state_q) AwaitPattern: begin - state_d = (transition_count_q == 4'he) ? AwaitSr : AwaitPattern; + state_d = (sda_transition_count_q == 4'he) ? AwaitSr : AwaitPattern; end AwaitSr: begin state_d = AwaitSr; - if (start_detected_i & scl_high) begin - state_d = AwaitP; - end else if (scl_low | suspicious_transition_count_q > 1) begin + // If SDA posedge is detected, it means that it toggled from LOW to HIGH before fulfilling + // the START condition hold timing, otherwise `start_detected_i` would be asserted + if (scl_low | sda_posedge) begin state_d = AwaitPattern; + end else if (start_detected_i) begin + state_d = AwaitP; end end AwaitP: begin state_d = AwaitP; - if (stop_detected_i & scl_high) begin + // If SDA negedge is detected, it means that it toggled from HIGH to LOW before fulfilling + // the STOP condition hold timing, otherwise `stop_detected_i` would be asserted + if (scl_low | sda_negedge) begin + state_d = AwaitPattern; + end else if (stop_detected_i) begin state_d = ResetDetected; - end else begin - if (invalidate_sr) - state_d = AwaitPattern; end end ResetDetected: begin @@ -110,7 +89,7 @@ module target_reset_detector end default: begin state_d = AwaitPattern; - end // Unreachable + end endcase end : target_reset_detection_fsm diff --git a/third_party/cocotbext-i3c b/third_party/cocotbext-i3c index 3df515c..67fbd61 160000 --- a/third_party/cocotbext-i3c +++ b/third_party/cocotbext-i3c @@ -1 +1 @@ -Subproject commit 3df515c127033a9361bacb7b5d7cbc0044eb9cac +Subproject commit 67fbd61d55af3ed7b74ce9c2483e0f49dc379228 diff --git a/verification/cocotb/block/ctrl_bus_monitor/test_bus_monitor.py b/verification/cocotb/block/ctrl_bus_monitor/test_bus_monitor.py index 06fdb73..c2415ea 100644 --- a/verification/cocotb/block/ctrl_bus_monitor/test_bus_monitor.py +++ b/verification/cocotb/block/ctrl_bus_monitor/test_bus_monitor.py @@ -34,6 +34,7 @@ async def count_high_cycles(clk, sig, e_terminate): await RisingEdge(clk) return num_det + def create_default_controller(dut: SimHandleBase) -> I3cController: return I3cController( sda_i=None, @@ -162,11 +163,9 @@ async def test_target_reset_detection(dut: SimHandleBase): cocotb.log.info("Performing basic target reset test with no configuration") e_terminate = cocotb.triggers.Event() - t_detect_target_reset = cocotb.start_soon(count_high_cycles( - dut.clk_i, - dut.target_reset_detect_o, - e_terminate - )) + t_detect_target_reset = cocotb.start_soon( + count_high_cycles(dut.clk_i, dut.target_reset_detect_o, e_terminate) + ) await i3c_controller.target_reset() await ClockCycles(dut.clk_i, 32) diff --git a/violations.waiver b/violations.waiver index 36cc7c5..4b2dfed 100644 --- a/violations.waiver +++ b/violations.waiver @@ -1,5 +1,6 @@ waive --rule=line-length --location="src/ctrl/i2c_controller_fsm.sv" waive --rule=line-length --location="src/ctrl/i2c_target_fsm.sv" +waive --rule=line-length --location="src/ctrl/bus_monitor.sv" waive --rule=line-length --location="src/i2c_phy_integration.sv" waive --rule=line-length --location="verification/block/i2c_phy_integration/i2c_phy_integration_wrapper.sv" waive --rule=line-length --location="src/csr/I3CCSR_covergroups.svh"