Tom's review comments uploaded

1abec1e7 · Tomasz Wlostowski · 41083c1c · 1abec1e7
Commit 1abec1e7 authored Nov 12, 2013 by Tomasz Wlostowski
Hide whitespace changes
Inline Side-by-side

Showing with 86 additions and 0 deletions

tom.txt design-review/tom.txt +86 -0

No files found.
--- a/design-review/tom.txt
+++ b/design-review/tom.txt
+CONV-TTL-BLO VHDL review
+T.Wlostowski
+------------------------
+
+[M] = minor, [O] = moderate, [S] = serious
+
+General:
+
+Documentation looks really cool, it's detailed, graphically pleasing and very 
+nicely written. Same for the code, clear and well commented. Well done!
+
+[M] Doc: when talking about clocks, mention that the complex systems 
+    of DACs and VCXOs is for White Rabbit,
+[M] Repository: please, no binary/PDF files in the repo, it takes 
+    already few hundred megabytes.
+[M] Repository: remove all old Carlos' code from the master branch, 
+    keep only the files that are actually used in the project.
+
+conv_ttl_blo.vhd (top level):
+
+[S] fpga_clk/clk125 is not constrained in the .ucf file. After constraining 
+    the main clock to 125 MHz, your design does not meet the timing. 
+    Pulse inputs that are driving clock pins of the FFs are not constrained too,
+    causing P&R timing errors.
+[M] Line 429, 504: avoid hardcoded constants
+[M] Process p_i2c_blink: it does a simple pulse width extension. Consider 
+    using gc_extend_pulse. Also, define the state as an enum, this way
+    you'll avoid ugly looking "others" case and improve readability.
+[M] Similar issue for p_pulse_leds process.
+[M] Line 809: I'm not sure if value 127 wouldn't cause an overflow in 
+    bicolor_led_driver. (it says 0-100)
+
+reset_gen.vhd:
+[M] Consider adding a software reset bit
+
+ctb_pulse_gen.vhd:
+    
+[O] With glitch filter off: the trigger "clocks in" the enable pin of 
+    the FF, which is used to control output pulse width. I couldn't understand 
+    the benefits of this method with respect to a simple AND gate. The 
+    FF created in p_pulse_gf_off process makes the P&R tools unhappy by
+    driving clock pins with non-clock nets.
+[M] With glitch filter on: looks fine, I would only put some extra comments 
+    explaining the paths of the glitchy and deglitched signals.
+[M] Marking async signals with "_a" suffix (e.g. trig_a_i) will make 
+    the code easier to analyze.
+
+i2c_slave.vhd
+[S] Lines 202+: make sure the glitch filter deals correctly with metastabilty 
+    in the I2C signals (see below).
+[M] Maybe the edge detection logic could be moved into glitch_filter 
+    module, saving ~50 lines of repetitive code?
+[S] According to the I2C specification, slaves should sample SDA on rising 
+    edge of SCL. The master must ensure enough setup/hold time of SDA 
+    around the rising edge of SCL.
+[S] In your code (lines 435, 468), both the tristate enable and output 
+    are changed at the same moment. This may cause glitches on the bus
+    caused by internal FPGA delays between the sda_o and sda_en_o signals 
+    (imagine a case where sda_o goes from 1 to 0 slightly after the sda_en_o 
+    was activated). It's sufficient to drive the output pin (sda_o) always 
+    low and force the required state with an open-drain-like output 
+    (setting sda_en_o to 1 for logic 0 on the I2C bus). I'm not sure if
+    open-drain drivers are not required by the I2C spec (need to confirm).
+
+glitch_filt.vhd:
+[M] The dat_i input is asynchronous (driven by trig_i signal in ctb_pulse_gen). 
+    Add "_a" suffix.
+[S] ...As a consequence, glitch_filt(0) is asynchronous wrs to the input 
+    clock, and glitch_filt(1) can be metastable, since it's the output 
+    of the 1st FF in the sync chain. Add an explicit synchronizer on dat_i 
+    signal before it gets into p_glitch_filt process or make the delay line longer
+    by 2 bits and ignore the last 2 bits in comparison (process p_output).
+
+vbcp_wb.vhd
+[O] Consider informing the I2C master about an error in a WB transaction 
+    (e.g. by using an extra bit)
+[S] Add an wb_ack/wb_err timeout. Currently accessing an unhandled address 
+    will hang the FSM.
+[S] Wire the I2C watchdog to the reset of the VBCP FSM or ensure another
+    watchdog mechanism. Even if the I2C core recovers through a hang thanks to its 
+    watchdog, the VBCP FSM may still stay frozen.
+
+multiboot_regs.vhd
+[O] Include the source .wb file in the repo.
+[O] Explain the changes you did do the wbgen-generated code. Perhaps it could be
+    added as a feature to wbgen?