Commit 6c090d60 authored by twlostow's avatar twlostow

Uploaded Tom's comments for the review

git-svn-id: http://svn.ohwr.org/fmc-tdc@70 85dfdc96-de2c-444c-878d-45b388be74a9
parent 9eebe296
Disclaimer. I didn't have too much time to review. Apologize for eventual typos or errors in my comments.
T.
General:
------------------
- Wait statements in synthesizable code look suspicious... They indeed synthesize correctly, but to be honest, I've never seen processes coded in such way.
- (googled a bit) Technically, it should be "wait until rising_edge(clk)" or "wait until (clk'event and clk = '1')".
- Are I/O always assigned to internal signals for some particular reason?
- I'd suggest declaring commonly used components (e.g. counters) in a shared package to avoid repetitive declarations.
- Clock signal assignments are dangerous. On a simulation, there is a big risk of getting timing errors when co-simulated with Verilog code due to incompatibilities between the way events are scheduled in VHDL and Verilog simulators (in VHDL, a continuous assignment is scheduled as an event, while in verilog it's purely continous).
acam_databus_interface:
-------------------------
- line 192: these signals (efX, lfX) are asynchronous. I'd suggest using a sync chain of 2-3 flip-flops.
acam_timecontrol_interface:
----------------------------
- line 89: the name doesn't explain the purpose of this constant. What delay does it describe?
- line 125: are you sure this will work correctly? err_flag_r(2) is written twice.
How about using slice & join instead?
err_flag_r <= err_flag_r(err_flag_r'left-1 downto 0) & err_flag_i;
- line 249: acam_refclk and clk are normally phase aligned by the AD9516 PLL, so there may be a setup time violation here. Is the AD9516 shifter programmed to ensure the FPGA will correctly sample acam_refclk signal? (otherwise, the 1st stage could sample on the falling edge of clk).
- line 255: refclk_r(3) can be metastable, causing refclk_edge signal to be unreliable. Consider adding 1 more sync stage (or using only (1) and (0) indices)
- line 260: the same for start_trig_edge
circular_buffer:
-------------------------
- pipelined WB is not that complex, there's no need for an FSM.
In case of a non-stalling peripheral (stall == 0 always), the ack signal can be generated like this:
process(clk)
ack <= stb and cyc;
(adr and dat go straight to the block ram).
- Consider replacing Coregen cores with generic ones. The circular buffer can be done as a simple array.
clk_rst_managr:
---------------------------
- line 166: IBUFDS + BUFG can be merged into single IBUFGDS
- line 225: chain of two global buffers on spec_clk_i (IBUFG drives a global clock net, so there's no need to follow it with another BUFG)
- lines 409+: I'd suggest defining these regs as an array of records?
- lines 289+: I couldn't understand the way the power-on-reset is generated. A comment would be greatly appreciated.
countdown_counter:
free_counter:
incr_counter:
-----------------------
- line 49, 52: (un)signeds can be compared with integers directly (numeric_std supports this).
if (value = 0) ...
- coding style (_i suffix for inputs, etc.).
data_engine:
-----------------
- state names look like signal names, consider using uppercase or prefixes to avoid confusion.
- line 294: is the others block ever reached?
- define addresses of commonly used ACAM regs as constants (e.g. c_ACAM_IFIFO1 for x"08", etc...) to improve readability.
one_hz_counter:
-----------------
- lines 161+: you're syncing the same signal (tdc refclock) twice in the design (here and in acam_timecontrol_interface). Due to possible metastability, you can get inconsistent pulses in these two modules.
reg_ctrl:
-----------------
- use constants for defining register adresses
- line 135+: avoid repetitive assignments. Use loop construct instead.
- line 126+: reg_ack <= reg_stb and reg_cyc and not reg_ack;
tdc_core_pkg:
---------------------
- lines 73+: consider defining these constants in decimal format (these are timeouts, and in decimal they are easier to understand).
top_tdc:
---------------------
- what are g_span and g_width generics (a comment would be helpful)
- gnum_reset signal is asynchronous, but used throughout the design as synchronous. Add a sync chain.
- put together all the components which form the TDC core into a single VHDL entity, with the ACAM I/F on one side and Wishbone on other side (i.e. without the gennum or other platform-specific stuff inside)
- lines 1024, 1039: when decoding addresses, define base addrs as constants instead of using hardcoded values
sim/
----------------
- Try to avoid uploading binary files if they are not absolutely necessary (i.e. compiled Xilinx libraries).
- A system-level testbench should be provided (tb_tdc.vhd doesn't include any actual testbench code, just the models connected together).
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment