Commit a86d35bc authored by penacoba's avatar penacoba

Comments gathered in summary


git-svn-id: http://svn.ohwr.org/fmc-tdc@71 85dfdc96-de2c-444c-878d-45b388be74a9
parent 6c090d60
Summary of all the comments to the code by authors.
General:
========
TOM
- 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).
top_tdc.vhd
===========
JAVIER
- Line 1057 (and others). The wait until spec_clk = '1'; at the end of
the process looks awkward to me. I guess it means this is a
synchronous process working on the rising edge of spec_clk. Is there
any advantage to using this notation?
TOM
- 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
acam_databus_interface.vhd
==========================
JAVIER
- Line 73. "read" and "write" are not VHDL reserved words but they are
names of functions people use to do I/O. Probably wise to chose
other names for states.
- Line 244. address_o going to the ACAM is not registered. This signal
is driven by adr_i, the wishbone input address. These lines are
being used in another entity (data_engine.vhd) as inputs to other
processes so chances are they will not use IOB FFs, which might be
important to respect setup and hold constraints of the ACAM.
TOM
- line 192: these signals (efX, lfX) are asynchronous. I'd suggest using a
sync chain of 2-3 flip-flops.
EVA
- lines 227-241: signals ef1/2, lf1/2 shouldn t be synchronized as well
(use one more DFF)?
- The ack signals are pulses (_p)
acam_databus_interface, ack, line 63
acam_timecontrol_interface.vhd
==============================
JAVIER
- Line 246 (and others). This can be written in one line:
"start_trig_r <= start_trig & start_trig_r(1 downto 0);"
- Line 255 (and others). This edge detector uses signal ref_clk_r(3),
which is potentially metastable. It also relies very heavily on the
fact that ref_clk should be at a given frequency.
TOM
- 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
EVA
- synchronization and edge detection of refclk takes place in both units
one_hz_gen lines 153-170; acam_timecontrol_interface lines 240-258
refclk_edge is a pulse (refclk_edge_p)
use of the first DFF s_acam_refclk(3) should be avoided
- lines 260: start_trig_edge is a pulses (_p)
use of the first DFF (start_trig_r(2)) should be avoided
- lines 120-146 : good!-)
clk_rst_managr.vhd
==================
JAVIER
- Line 284. This process can create metastability in signal gral_incr,
which is then going to many destinations inside the incr_counter
block, possibly leading to non-deterministic behavior of the counter.
- Line 309. Signal cs seems to be negative logic. This should be visible
in its name.
- Line 571. cs will not use an IOB FF because it is read in line
366. Please check all other cases when this can happen.
TOM
- 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.
one_hz_gen.vhd
==============
JAVIER
- Line 153. Similar comments on metastability as before. Also, the
assumption on the clock frequency of s_acam_refclk is very strong and
might be in trouble if sampling happens close to the edges and the
signals are a bit jittery. Why is this "frequency test" needed?
TOM
- 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.
EVA
synchronization and edge detection of refclk takes place in both units
one_hz_gen lines 153-170; acam_timecontrol_interface lines 240-258
refclk_edge is a pulse (refclk_edge_p)
use of the first DFF s_acam_refclk(3) should be avoided
data_engine
===========
TOM
- 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.
EVA
- I would suggest a bit cleaner names for the WBs:
stb/ack/.. in the data_engine could be renamed to
acam_stb/ acam_ack..
circular_buffer
===============
TOM
- 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.
EVA
- I would suggest a bit cleaner names for the WBs:
classic_stb/ classic_ack/.. in the circular_buffer renamed to
gnum_classic_stb/ gnum_classic_ack..
pipe_stb/ pipe_ack/.. in the circular_buffer renamed to
gnum_pipe_stb/ gnum_pipe_ack..
- lines 37, 50: class_clk_i and pipe_clk_i are actually the same clock;
maybe they could just be named gnum_clk_i
- The ack signals are pulses (_p)
circular_buffer, classic_ack, line 123
countdown_counter:
free_counter:
incr_counter:
=============
TOM
- 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.).
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).
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