Add glitch filters to I^2C inputs #285
Conversation
There was a problem hiding this comment.
Thanks @elliotb-lowrisc this LGTM modulo some coding style nits.
On the meta-stability point we do have a synchroniser in the i2c top-level
sonata-system/vendor/lowrisc_ip/ip/i2c/rtl/i2c_core.sv
Lines 431 to 450 in 49e57ca
So I'd be tempted to remove that synchronizer and implement it inside here instead (i.e. apply prim_flop_2sync to raw_i in this module and then use the output of the synchronizer everywhere you currently use raw_i). I'm a little nervous as to what a real glitch would do if it causes metastability here and given it gets synced eventually may as well push that upstream to here. What we'd like to avoid is repeating the synchronization though. I think in OT this filter is implemented via an analogue component, something like a Schmitt Trigger in the pad so it doesn't have the same metastability concerns.
It's a little fiddly to do this as you'll need to adjust the vendoring patch files and rerun the vendor script, @marnovandermaas or @alees24 can advise.
rtl/system/i2c_filter.sv
Outdated
| // Width counter. Implement as shift register to keep logic | ||
| // on the path from the counter to the datapath to a minimum. | ||
| logic [FilterCycles-1:0] shift_counter_plus1; | ||
| logic [FilterCycles-1:0] shift_counter; |
There was a problem hiding this comment.
Please use _d and _q suffixes. Here you'd have shift_counter_q in place of shift_counter and shift_counter_d in place of shift_counter_plus1. The always_ff block below teeters on the edge of acceptable complexity for an always_ff block (see https://github.com/lowRISC/style-guides/blob/master/VerilogCodingStyle.md#sequential-logic-registers) but I think I'm happy to leave it as is because it makes the shift_counter_d logic simpler.
rtl/system/i2c_filter.sv
Outdated
| logic [FilterCycles-1:0] shift_counter; | ||
|
|
||
| // MUX select signal | ||
| logic select; |
There was a problem hiding this comment.
I'd make this a more descriptive name select_raw maybe?
rtl/system/i2c_filter.sv
Outdated
| // MUX select signal | ||
| logic select; | ||
| // Internal filter output signal | ||
| logic result; |
There was a problem hiding this comment.
It'd be better to unify the names here, we've got prev_out, result and filtered_o all effectively referring to the same bit, either the immediate combinational version of that bit or the flopped version.
Perhaps switch to filtered_o, filtered_q (previously prev_out) and filtered_d (previously result).
rtl/system/i2c_filter.sv
Outdated
| if (FilterCycles == 1) begin | ||
| // Only one bit, so no shifting | ||
| assign shift_counter_plus1 = 1'b1; | ||
| end else if (FilterCycles == 2) begin | ||
| // Two bits, so only one bit gets shifted | ||
| assign shift_counter_plus1 = {shift_counter[0], 1'b1}; | ||
| end else begin | ||
| // Many bits, so a range get shifted | ||
| assign shift_counter_plus1 = {shift_counter[FilterCycles-2:0], 1'b1}; | ||
| end |
There was a problem hiding this comment.
Please add names to the generate blocks, e.g.
| if (FilterCycles == 1) begin | |
| // Only one bit, so no shifting | |
| assign shift_counter_plus1 = 1'b1; | |
| end else if (FilterCycles == 2) begin | |
| // Two bits, so only one bit gets shifted | |
| assign shift_counter_plus1 = {shift_counter[0], 1'b1}; | |
| end else begin | |
| // Many bits, so a range get shifted | |
| assign shift_counter_plus1 = {shift_counter[FilterCycles-2:0], 1'b1}; | |
| end | |
| if (FilterCycles == 1) begin : g_one_filter_cycle | |
| // Only one bit, so no shifting | |
| assign shift_counter_plus1 = 1'b1; | |
| end else if (FilterCycles == 2) begin : g_two_filter_cycles | |
| // Two bits, so only one bit gets shifted | |
| assign shift_counter_plus1 = {shift_counter[0], 1'b1}; | |
| end else begin : g_many_filter_cycles | |
| // Many bits, so a range get shifted | |
| assign shift_counter_plus1 = {shift_counter[FilterCycles-2:0], 1'b1}; | |
| end |
|
I should perhaps have thought of this sooner, but we do vendor in the primitive |
|
Ah yes I hadn't realised that existed! Apologies @elliotb-lowrisc but I think we're best off using the existing primitive, sorry for the wasted design time. Would you mind incorporating |
|
Righto, I'll have a go at the |
I^2C Fast-mode requires a 50 ns glitch filter on SCL and SDA inputs. This is an attempt at implementing such a filter using the existing prim_filter IP. Note, as it is operating in the digital domain it could fall victim to inexact clock division. Happily the clock divides nicely at our current 40 MHz system clock.
elliotb-lowrisc
force-pushed
the
i2c_filter
branch
from
51e4dcc
to
ea4328c
Compare
|
Reworked to use |
|
This PR is currently causing my (2021.1) builds to fail timing by a considerable margin for some unknown reason. I shall attempt to investigate. |
|
Timing seems fine now |
|
Notable increase in area utilisation observed from this PR. Seems to be related to an I^2C distributed memory no longer being inferred, which may be related to some signals being merged that were not before. Will revisit after 1.0 release. |
I^2C Fast-mode requires a 50 ns glitch filter on SCL and SDA inputs. This is an attempt at implementing such a filter.
Note, as it is operating in the digital domain and on an unregistered input, it could fall victim to inexact clock division and metastability. Happily the clock divides nicely at our current 40 MHz system clock.
So far only tested in simulator in ideal conditions, not on real hardware nor exposed to glitches.