Comments on I2C Slave core

47f0c8b2 · Javier Serrano · e1851be4 · 47f0c8b2
Commit 47f0c8b2 authored Nov 07, 2013 by Javier Serrano
Show whitespace changes
Inline Side-by-side

Showing with 45 additions and 0 deletions

javier.txt design-review/javier.txt +45 -0

No files found.
--- a/design-review/javier.txt
+++ b/design-review/javier.txt
@@ -136,6 +136,51 @@ see a constant called "c_pulse_width_gf_off" I expect it to hold the
 pulse width, not some other value which is needed in the state machine
 so the actual pulse width turns out to be g_pwidth.

+i2c_slave.vhd
+=============
+
+Line 259: strange constructs to generate sda_rising and sda_falling. I
+had seen that kind of construct used with variables, but never with
+signals. Are you sure this generates one-tick-long pulses? Typically
+people use if-then-else for this, or even simpler: 
+sda_rising <= sda_degl and not sda_degl_d0;
+
+It would be easier to see if there are any missing assignments of
+outputs in the state machine if all output assignments happened in
+consecutive lines and always in the same order.
+
+Bear in mind that if you have a glitch in one of the lines (SCL or
+SDA) and not in the other one, the relative timing of these two
+signals will be perturbed for that particular bit. What is the margin
+you have for misalignment in the case of the I2C master in the ELMA
+crates?
+
+You seem to be clocking in bits from SDA upon detection of a falling
+edge in SCL. Isn't this a bit risky? What I understood from the I2C
+document is that the data line is supposed to be stable while SCL is
+high. Why not sample the SDA line a few ticks before the falling edge?
+This could be done with a tapped delay line on SDA. Your system clock
+is way faster than any I2C link you can imagine, so a few ticks (and a
+corresponding note in the user manual) should be safe.
+
+A bit of an aesthetic point: isn't the safe value for sda_o '1', in
+terms of acknowledgement of transfers? If so, shouldn't the default
+value in the IDLE state be '1'?
+
+The watchdog machinery is quite intricate. I'd go for something
+simpler if possible. You are embedding the generation of the watchdog
+reset signal in the very FSM you want to help with it. Imagine the
+following scenario: you enter the ST_ADDR state, then a first
+scl_falling comes and no more. This first scl_falling sets watchdog
+reset to '1', and then you stay stuck in ST_ADDR forever. The watchdog
+should ideally look at how much time you're spending outside the IDLE
+state and take you there if it is too long. Optionally, you could also
+look at how long you're spending in each state. But IMHO it should be
+a process completely *outside* the state machine logic. It would also
+be good for the watchdog to leave some kind of diagnostics trace when
+it fires, so you can identify possible design errors which might go
+unnoticed otherwise.
+
 Todo
 ====