Conversation
- Adding reset synchronizers to avoid non-deterministic deadlock - Making clk0 the main clockpin that must always toggle, default during reset - Adding comments
WalkthroughThis PR adds an async active-low reset ( Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lambdalib/auxlib/la_clkmux2/testbench/tb_la_clkmux2.v (1)
82-84: Consider asserting failure when output unexpectedly toggles.When
sel=1andclk1is stopped, the output should stop toggling. Currently this only logs informational output without failing the test ifout_edges > 0.🔧 Proposed assertion
out_edges = 0; `#200`; + if (out_edges != 0) begin + $display("FAIL: sel 0->1, clk1 stopped (expected 0 edges, got %0d)", out_edges); + $finish; + end $display("INFO: sel 0->1, clk1 stopped (%0d edges, expect 0)", out_edges);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lambdalib/auxlib/la_clkmux2/testbench/tb_la_clkmux2.v` around lines 82 - 84, The testbench currently only prints info when sel is set to 1 and clk1 is stopped but does not fail the test if the output toggles; after the block that sets out_edges = 0; waits `#200`; and $display(...), add an explicit assertion/failure check that triggers if out_edges > 0 (use $fatal or $error followed by $finish) so the test fails when the mux output unexpectedly toggles; reference the testbench signals/variables out_edges, sel, clk1 and the testbench module tb_la_clkmux2 to locate where to insert the check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lambdalib/auxlib/la_clkmux2/testbench/tb_la_clkmux2.v`:
- Around line 82-84: The testbench currently only prints info when sel is set to
1 and clk1 is stopped but does not fail the test if the output toggles; after
the block that sets out_edges = 0; waits `#200`; and $display(...), add an
explicit assertion/failure check that triggers if out_edges > 0 (use $fatal or
$error followed by $finish) so the test fails when the mux output unexpectedly
toggles; reference the testbench signals/variables out_edges, sel, clk1 and the
testbench module tb_la_clkmux2 to locate where to insert the check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 01500704-e108-48a4-aed4-9612b263343a
📒 Files selected for processing (4)
lambdalib/__init__.pylambdalib/auxlib/la_clkmux2/la_clkmux2.pylambdalib/auxlib/la_clkmux2/rtl/la_clkmux2.vlambdalib/auxlib/la_clkmux2/testbench/tb_la_clkmux2.v
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lambdalib/auxlib/la_clkmux2/testbench/tb_la_clkmux2.v (1)
104-112: Consider adding verification during the clk1 stop phase.The test verifies recovery after
clk1restarts but doesn't explicitly check that output stopped during the#100interval whenclk1was disabled. Adding a quick edge count check would strengthen this test case.🧪 Proposed enhancement
check_toggling("sel=1, clk1 running"); clk1_en = 0; clk1 = 0; + out_edges = 0; `#100`; + if (out_edges != 0) begin + $display("FAIL: sel=1, clk1 stopped (expected 0 edges, got %0d)", out_edges); + $finish; + end clk1_en = 1; check_toggling("sel=1, clk1 stopped then restarted");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lambdalib/auxlib/la_clkmux2/testbench/tb_la_clkmux2.v` around lines 104 - 112, Add an explicit verification that the output stopped when clk1 was disabled: after setting sel = 1 and clk1_en = 0 (and forcing clk1 = 0) insert a short edge-count / no-toggle assertion for the `#100` stop window (e.g. call an existing monitor or a new helper like check_no_toggling("sel=1, clk1 stopped") that asserts no edges occurred) before re-enabling clk1 and calling check_toggling again; reference sel, clk1_en, clk1 and check_toggling (or add check_no_toggling) to locate where to insert this check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lambdalib/auxlib/la_clkmux2/testbench/tb_la_clkmux2.v`:
- Around line 76-84: The INFO-only check for the stopped-clock scenario must be
turned into a real assertion so failures fail the test; after the block that
sets clk1_en=0, clk1=0, waits, sets sel=1 and measures out_edges (the code
around the variables clk1_en, clk1, sel, out_edges), add a conditional that
checks out_edges !== 0 and invokes a simulator-fatal/error (e.g., $fatal or
$error + $finish) with a descriptive message so the test fails when edges are
observed instead of only printing INFO; place this assertion immediately after
the existing $display for the sel 0->1, clk1 stopped case.
---
Nitpick comments:
In `@lambdalib/auxlib/la_clkmux2/testbench/tb_la_clkmux2.v`:
- Around line 104-112: Add an explicit verification that the output stopped when
clk1 was disabled: after setting sel = 1 and clk1_en = 0 (and forcing clk1 = 0)
insert a short edge-count / no-toggle assertion for the `#100` stop window (e.g.
call an existing monitor or a new helper like check_no_toggling("sel=1, clk1
stopped") that asserts no edges occurred) before re-enabling clk1 and calling
check_toggling again; reference sel, clk1_en, clk1 and check_toggling (or add
check_no_toggling) to locate where to insert this check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fbaa4507-a3af-42aa-9df4-1ddb4d10a7a7
📒 Files selected for processing (1)
lambdalib/auxlib/la_clkmux2/testbench/tb_la_clkmux2.v
| // 5. sel 0->1 with clk1 stopped (output should stop) | ||
| clk1_en = 0; | ||
| clk1 = 0; | ||
| #50; | ||
| sel = 1; | ||
| #100; | ||
| out_edges = 0; | ||
| #200; | ||
| $display("INFO: sel 0->1, clk1 stopped (%0d edges, expect 0)", out_edges); |
There was a problem hiding this comment.
Missing assertion for stopped clock scenario.
This test expects out_edges == 0 when sel=1 and clk1 is stopped, but only displays INFO without asserting the condition. A bug causing unexpected output toggling would go undetected.
🧪 Proposed fix to add assertion
out_edges = 0;
`#200`;
+ if (out_edges != 0) begin
+ $display("FAIL: sel 0->1, clk1 stopped (expected 0 edges, got %0d)", out_edges);
+ $finish;
+ end
$display("INFO: sel 0->1, clk1 stopped (%0d edges, expect 0)", out_edges);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 5. sel 0->1 with clk1 stopped (output should stop) | |
| clk1_en = 0; | |
| clk1 = 0; | |
| #50; | |
| sel = 1; | |
| #100; | |
| out_edges = 0; | |
| #200; | |
| $display("INFO: sel 0->1, clk1 stopped (%0d edges, expect 0)", out_edges); | |
| // 5. sel 0->1 with clk1 stopped (output should stop) | |
| clk1_en = 0; | |
| clk1 = 0; | |
| `#50`; | |
| sel = 1; | |
| `#100`; | |
| out_edges = 0; | |
| `#200`; | |
| if (out_edges != 0) begin | |
| $display("FAIL: sel 0->1, clk1 stopped (expected 0 edges, got %0d)", out_edges); | |
| $finish; | |
| end | |
| $display("INFO: sel 0->1, clk1 stopped (%0d edges, expect 0)", out_edges); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lambdalib/auxlib/la_clkmux2/testbench/tb_la_clkmux2.v` around lines 76 - 84,
The INFO-only check for the stopped-clock scenario must be turned into a real
assertion so failures fail the test; after the block that sets clk1_en=0,
clk1=0, waits, sets sel=1 and measures out_edges (the code around the variables
clk1_en, clk1, sel, out_edges), add a conditional that checks out_edges !== 0
and invokes a simulator-fatal/error (e.g., $fatal or $error + $finish) with a
descriptive message so the test fails when edges are observed instead of only
printing INFO; place this assertion immediately after the existing $display for
the sel 0->1, clk1 stopped case.
aolofsson
changed the title
1 participant
Making clockmux safer
User still has to be careful at the system side to make sure the sel signal and clk1 signals aren't sitting on logic clocked by the out clock. If it does, you could put system in a state where only reset will recover.