Skip to content

M
MasterFIP - Gateware
Commit 936d742a authored by Evangelia Gousiou's avatar Evangelia Gousiou
Browse files
Options

Merge branch 'eva_dev' of ohwr.org:cern-fip/masterfip/masterfip-gw into eva_dev

parents 76d20a64 6a3520b7
Expand all Hide whitespace changes
Inline Side-by-side
Showing with 490 additions and 0 deletions
doc/review_02032017/denia_comments.txt 0 → 100644
View file @ 936d742a
masterFIP review comments:
General comments:
==================
- Very readable code, useful comments, and very useful diagrams at the top.
- Repo seems to be in good order.
- Most processes are missing the p_ prefix.
- Optionally, use the name of the process at the end, i.e.: end process p_process_name.
- Indentation at some places needs to be corrected.
fmc_massterFIP_core.vhd
========================
- Line 517, 534 and 549: Expression core_rst = '1' or rx_rst = '1' used in 3 places.
Use one signal assignement and use that instead for optimisation.
- Same for core_rst_n = '0' or fd_host_rst = '1' which is used in various places.
- Some indentation may be improved, eg in: cmp_masterfip_tx.
- fd_wdgn signal should be renamed fd_wdg_n, same for fd_rstn_o-> fd_rst_n_o
- I know chipscope is commented, but, the port map has the wrong syntax
(<= instead of => and the semicolons at the end)
masterfip_tx:
=============
- I found the suffixes to express 1-tick delays a little confusing. You
have in the same process:
line 304 last_data_byte_p_d <= last_data_byte_p_tmp;
last_data_byte_p <= last_data_byte_p_d;
And this:
line 307
byte_request_accept_p_d1 <= byte_request_accept_p_tmp;
byte_request_accept_p_d2 <= byte_request_accept_p_d1;
Why not use the same suffix scheme? preferably use "_dn" for both sets of expressions,
in agreement with VHDL guidelines table 3 p12. "_tmp" can be replaced by
"_d0"?
masterfip_rx:
=============
- s_ suffix for signals is optional. I think its use is especially
redundant since it is not used for all the signals in this module.
- Line 270: create_32bit_words process name should have the p_ prefix.
- Line 300: use p_ prefix for process name
- Line 263: bytes_c_rst expression:
bytes_c_rst <= '1' when (rst_i = '1' or rx_rst_i = '1') else '0';
effectively means that bytes_c_rst is the same value as (rst_i = '1' or rx_rst_i = '1').
Make it as an assignement (?)
bytes_c_rst <= rst_i or rx_rst_i;
- The expression (rst_i = '1' or rx_rst_i = '1') is evaluated at 4
separate occasions. Would it be better to assign it to a signal and
perform the OR only once? (ties in with previous remark).
- In process data_transfer_to_regs:
- add p_ prefix to process name.
- The operation: word32_num <= word32_num + 1;
seems to be performed no matter what the condition is (apart from reset condition)
could be performed once straight after the first else?
- In...
.....
if word32_num = 0 then
word32_num <= word32_num + 1;
rx_frame_o(word32_num) <= byte0 & byte1 & byte2 & byte3;
elsif (unsigned(rx_byte_index)-2) mod 4 = 3 then -- [CRC|CRC|BYTE|BYTE]
word32_num <= word32_num + 1;
rx_frame_o(word32_num) <= byte0 & byte1 & byte2 & byte3;
.....
It looks like the two conditions could be grouped under one condition
with an OR as the outcome is the same. In fact, it seems that most
outcomes are the same apart from Line 338. It seems the code could be
much more compact.
leds_manager.vhd
=================
Could this core be made more generic by removing all the TDC-specific
references in comments and port names and moved inside general core ?
- Line 96: add prefix g_ for generic values_for_simul
- In process pulse_generator, all acam_channel(2 downto 0) values other
than 0, 1, 2, 3 result in ch5 LED being lit. Is this okay functionally?
Wouldn it better to add the "="100" condition and send all the other
possibilities to the final else statement (all channels off?)
decr_counter.vhd
=================
- Use c_ prefix for constants.
- Couldn't signal one <= zeroes + "1" be made into a constant, something like:
constant c_one : unsigned (width-1 downto 0):= to_unsigned (1, width-1) (?)
- Is counter_is_zero_o supposed to be 1-tick long? if so add _p suffix,
else it could be assigned outside the process like this?
counter_is_zero_o <= '1' when counter = zeroes else "0";
- Also seems like since decrement operation is present in most
conditions, the code could be made more compact.
incr_counter.vhd
=================
- s_counter does not need prefix.
doc/review_02032017/dlamprid_comments.html 0 → 100644
View file @ 936d742a
This diff is collapsed.
doc/review_02032017/dlamprid_comments.org 0 → 100644
View file @ 936d742a
# emacs org-mode options and definitions, just ignore
#+OPTIONS: toc:nil
#+OPTIONS: ^:nil
#+TITLE: Comments for masterFIP GW review <2017-03-16 Thu>
* Project/Synthesis/PAR
- [+] 3943 warnings during synthesis
- [+] ucf: "TS_U_Node_Template_U_GN4124_Core_cmp_clk_in_feedback" points to non-existing
node 'U_Node_Template/U_GN4124_Core/cmp_clk_in/feedback'.
- [-] 120+ warnings about missing/duplicates files when we first open the .xise project file, and
the messages keep reappearing while we use the tool
- [-] Project hierarchy shows too many unused files (not under spec_masterfip_mt entity)
- [-] running hdlmake in syn/spec does not work
- [-] many (~50) synthesis-generated files not git-ignored
- [-] if not a build script for wbgen, at least a README with how you expect the wbgen2 command to
be invoked should be provided
* Simulation
- [!] Error: (vcom-19) Failed to access library 'nanofip_lib' at "nanofip_lib" (Modelsim SE-64
10.2a, Linux) This is due to case-sensitivity in Linux. Solution: replace "vlib nanoFIP_lib"
with "vlib nanofip_lib" or, alternatively, replace "vcom -work nanofip_lib" with "vcom -work
nanoFIP_lib"
- [!] Error: (vcom-7) Failed to open design unit file "../../sim/spec/testbench/nanofip_lib/*" in
read mode. Again, this is due to case-sensitivity in Linux, since the folder commited in git is
actually sim/spec/testbench/nanoFIP_lib
- [+] Trying to step into the code with Modelsim produces: Error opening
/opt/Xilinx/14.7/ISE_DS/ISE/vhdl/src/unisims/primitive/PLL_ADV.vhd. This path does not exist in
my system.
- [+] Fix warnings "numeric_std.to_integer metavalue detected, returning 0" (alternatively, use
"set NumericStdNoWarnings 1" after the call to vsim in your do files, suboptimal solution because
it might also hide useful metavalue warnings)
- [-] After clearing the metavalue warnings, have a look also at the remaining warnings
- [-] Some signals are always 'X', if they are not useful in simulation, just remove them
- [-] group signal waveforms
- [-] split compilation (vcom) from running in separate "do" files, invoke them both from a top do
file (eg. compile.do + run.do => sim.do)
- [-] many (~100) simulation-generated files not git-ignored
- [?] Do you need the "vlog $env(XILINX)/verilog/src/glbl.v"? If not, remove it, otherwise users
might get the error "can't read "env(XILINX)": no such variable" if they haven't set this
environment variable.
* Design
- [?] rtl: is leds_manager.vhd used at all? what about carrier_info.vhd, free_counter.vhd, and
perhaps others? If not, it's better to delete them from the repository.
- [?] rtl: why is wf_package.vhd declared here AND in ip-cores/nanofip?
- [?] top: is synthesis_descriptor.vhd used?
- [*] the whole nanofip as ip-core of masterfip seems a bit counter-intuitive. If there are things
used by both, the should belong to a "fip" project, and both nano- and master- should use them.
** masterFIP_pkg
- [-] constants should have lower case "c" (I think)
** spec_masterfip_mt
*** fmc_masterFIP_core
- [-] it would be nice to have a bit more hierachy in this module, with less low-level processes
and modules lying around (eg. counter modules, assignments to wb registers) , to highlight the
high-level structure of the core.
- [?] why do you use two decreasing counter modules (wf_decr_counter and decr_counter)?
- [?] why is speed_X_i not a 2-bit vector?
- [?] would it be interesting for diagnostics to monitor if/when counters overflow?
- [?] why is reg_to_mt.fd_wdg_tstamp_i one bit larger than macrocyc_cnt?
- [*] cmp_ext_sync_deglitch_p_detect and cmp_fd_wdgn_deglitch_p_detect introduce one unnecessary
FF/latency cycle
- [*] we should introduce generic up/down counters to general-cores
**** masterfip_tx
- [-] synch_signals process could be replaced by 2x gc_sync_ffs
- [?] more importantly, why do you resync these two signals?
**** masterfip_rx
- [?] can't you use general-cores for cmp_rx_deglitcher? (perhaps think about it also in nanofip
project)
- [?] did you make up your mind about what to connect to nfip_rst_i port of cmp_rx_deglitcher? if
yes, remove the inline comment
* Legend
- [!] = fatal
- [+] = important
- [-] = minor
- [?] = question
- [*] = note
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