Commit 73627664 authored by Tristan Gingold's avatar Tristan Gingold

Add reviews.

parent 557e12cf
- top VME64x core entity is called "VME64xCore_Top" while
svec/hdl/top/vmecore_test/ instantiates vme64xcore_top. Make it the same (all
lower-case?) for easier grepping.
- check comments in all the files, some of them are not complete or out of date
- shall we have some reference design in the VME64x core repo? With necessary
constraints for the clock and putting registers in IOBs (like vme_irq_n_o in
svec vmecore_test)
- vme64x_pack.vhd -> we usually call these things *_pkg.vhd
- from files headers remove _last changes_ and _TODO_ sections, anyway, they are
- i'd prefer to have signals without "s_" prefix, but this is part of a larger
coding style discussion.
In this design most of the signals are with "s_" prefix, but some are also
without (e.g. in VME_IRQ_Controller.vhd)
- how about using t_vme64x_in and t_vme64x_out types inside the design to make
the core more compact and having only an std_logic wrapper for simulations?
- vme64x_pack.vhd and xvme64x_core_pkg.vhd should be merged into one package
-- vme64x_pack.vhd --
- line 55:
c_ADEM_M is a type not a constant, rename it to t_ADEM_M
- cleanup, remove types that are not used e.g. c_ADER_C_XAM or c_ADER_C_AM
-- VME64xCore_Top.vhd --
- prefix Wishbone ports with "wb_"
-- VME_bus.vhd --
- prefix Wishbone ports with "wb_"
- constant num_latchDS misses "c_" prefix
- line 431: 3 nested if-s. How about simplifying to:
if decode_done_i = '1' and decode_sel_i = '1' and module_enable_i = '1'then
elsif decode_done_i = '1' then
s_mainFSMstate <= WAIT_END;
s_mainFSMstate <= DECODE_ACCESS;
end if;
- line 461:
should you check if s_DS_latch_count >0 before decrementing it by 1?
if for whatever reason you stay longer in DECODE_ACCESS waiting for
decode_done_i='1' you might end up going to WAIT_FOR_DS when your counter is
already 0, then after your decrement your counter rolls over
-- VME_CR_CSR_Space.vhd --
- line 323:
can use xor_reduce(vme_ga_i) from std_logic_misc instead of manually xoring
all the bits
-- VME_Funct_Match.vhd --
- line 94:
That's a weird way of describing registers in the process (at least to me).
Instead of clearing the registers at every risinge_edge, and having null in
rst_n_i='0', please clear the registers when rst_n_i ='0' and add another
"else" if you want to clear them every time decode_start_i='0'.
- line 133:
What's this construction??:
mask(c_ADEM_M) := g_ADEM(s_function_sel)(c_ADEM_M);
where c_ADEM_M is a subtype is integer range 31 downto 8;
-- VME_IRQ_Controller.vhd --
- reset_n_i, we always name it rst_n_i
- line 170:
very very tiny thing, retry_count can be cleared always when we're in
WAIT_IRQ (not only inside the _if_). This simplifies an already simple fsm.
This diff is collapsed.
* General notes
- pull dlamprid-vme64x_review for both the core and the SVEC demo (vmecore_test)
+ adjust/merge as you see fit
- [Update]/[Delete] top-level README.txt?
- [Update] HDL headers
- [Update]/[Delete]/[Move to user manual] header comments with technical explanations?
- Good opportunity to rename files, entities and instances. eg:
+ pack or pkg?
+ capital or small case file names?
+ ...
- does not strictly follow proposed folder structure
* VME64x core
- get rid of SVEC_ID, CERN_ID etc from package and default generic values
- what happens if the c_CLOCK_PERIOD is wrong? maybe it's better to get rid of it, set g_CLOCK_PERIOD by default to zero and assert that it is set to something else by user
- shal we name/label all processes?
** VME64xCore_top.vhd
- Add generic/constant for number of supported functions and use it to limit size of ader/adem
arrays etc, in order to reduce number of warnings in ISE (from ~450 to ~50)
- gc_sync_register: does it even make sense? Slide #95 of NASA radiation tolerant design presentation:
*** VME_CR_CSR_Space.vhd
- delete unused port vme_sysfail_ena_o?
* Simulation
- how am I supposed to run it? A README would help
- why open the GUI at the end of make?
- why keep around VME BFM if you are not using it? Why the SVEC-specific file in the VME BFM?
- why not (also) GHDL if Modelsim is not necessary?
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