lowRISC / lowRISC/opentitan

[alert,rtl] The `skew_cnt_q` signal is bigger than needed in prim_diff_decode.sv

Open
#29,170 4 comments 0 reactions 0 assignees View on GitHub
Component:RTL IP:alert_handler
Dominant language
SystemVerilog
Stars
3.6k
Forks
1.1k
Avg merge
2d 22h
Merged PRs (30d)
141

Description

### Description

The `prim_diff_decode` module has a counter called `skew_cnt_q` that is used to allow the signal to be non-differential for a short time (`SkewCycles`) before the mismatch is upgraded from a "skew" to a "sigint failure". The signal is quite new: it was only added in July 2025 (in commit cb26912606e) to support a use-case that wanted to tolerate skews of more than a single cycle.

I think the counter is bigger than it needs to be, though. It gets set to `1` (to represent the cycle where things have gone wrong) as we leave the `IsStd` FSM state and I don't think it can ever be zero in FSM state `IsSkewing`. It gets zeroed again as we enter `IsStd` or `SigInt`.

So I suggest "subtracting one" from the value in the only state we care about it:
- Don't set `skew_cnt_d` as we leave `isStd`
- Change the comparison in `IsSkewing` to something like `skew_cnt_q < SkewCycles - 1`

Doing so might let the signal get smaller. At the moment, it's declared as:
```
// Counter for skew cycles tolerated before flagging an issue
// The width needs to accommodate SkewCycles + 1 to count up to SkewCycles.
logic [prim_util_pkg::vbits(SkewCycles + 1)-1:0] skew_cnt_d, skew_cnt_q;
```
but the change would allow
```
logic [prim_util_pkg::vbits(SkewCycles)-1:0] skew_cnt_d, skew_cnt_q;
```
instead. If `SkewCycles` is a power of two, this will matter. For the parameterisation in `EarlGrey`, `SkewCycles = 1`, so the two counters will change from 2 bits to 1 bit each.

It won't make any difference to Darjeeling, where `SkewCycles = 3`.

I should note that I found this by looking at formal coverage numbers for the async alert testbench, where Jasper correctly pointed out that it could never make the comparison on `prim_diff_decode.sv:162` false. Congratulations to the tool vendors!

Contributor guide

Open the contributing guide

Research direction

Start in prim_diff_decode.sv at the skew_cnt_d logic and the comparison near line 162; review the IsStd and IsSkewing transitions first. Check the async alert testbench's formal coverage, and confirm the reduced counter width behaves for EarlGrey (SkewCycles=1) and Darjeeling (SkewCycles=3).

Written by the indexing model from the issue text.

Assessment

Domain
embedded-iot, security
Issue type
Refactor
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
64/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.