Skip to content

V
VME64x core
Commit 131c5766 authored by Tristan Gingold's avatar Tristan Gingold
Browse files
Options

Update review document after 2017-11-13 review.

parent 34239178
Hide whitespace changes
Inline Side-by-side
Showing with 109 additions and 54 deletions
doc/review-2017-11-13/GD-vme64x-core-review.txt
View file @ 131c5766
- 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.
OK.
- check comments in all the files, some of them are not complete or out of date
OK.
- 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)
Points to the svec repo.
- vme64x_pack.vhd -> we usually call these things *_pkg.vhd
OK
- from files headers remove _last changes_ and _TODO_ sections, anyway, they are
empty.
OK.
- 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)
'Detail'.
- 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?
Good convention to be adopted.
- vme64x_pack.vhd and xvme64x_core_pkg.vhd should be merged into one package
OK.
------------------------
-- vme64x_pack.vhd --
......@@ -23,29 +31,39 @@
rename c_CLOCK_PERIOD to something like c_default_CLK_PERIOD
because as far as I've seen in the code, that's what it is, a default clk
period value assigned to g_CLOCK_PERIOD if user does not overwrite it.
Default to be removed.
- line 55:
c_ADEM_M is a type not a constant, rename it to t_ADEM_M
Add a comment.
- cleanup, remove types that are not used e.g. c_ADER_C_XAM or c_ADER_C_AM
Add a comment as unused.
------------------------
-- VME64xCore_Top.vhd --
------------------------
- if g_WB_DATA_WIDTH must be 32-bit, then don't make it a generic, just use
everywhere c_DATA_WIDTH declared in the package
OK: to be removed.
- the same for g_WB_ADDR_WIDTH (see also comment for VME_bus.vhd)
OK: to be removed.
- prefix Wishbone ports with "wb_"
OK.
- if you make VME_bus.vhd with reset active low, then you don't need s_reset
signal anymore (see also comment in VME_bus.vhd).
OK.
------------------------
-- VME_bus.vhd --
------------------------
- prefix Wishbone ports with "wb_"
OK.
- either fix adr_o handling or remove g_WB_ADDR_WIDTH because setting anything
else than 32-bit to g_WB_ADDR_WIDTH will result in synthesis/simulation error
OK.
- why reset is active high (rst_i) and not active low like in all other modules?
OK.
- constant num_latchDS misses "c_" prefix
OK.
- 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
(...)
......@@ -54,18 +72,21 @@
else
s_mainFSMstate <= DECODE_ACCESS;
end if;
'As you like'.
- 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
OK.
--------------------------
-- 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
No, because std_logic_misc is not standard.
--------------------------
-- VME_Funct_Match.vhd --
......@@ -75,21 +96,26 @@
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'.
OK.
- line 133:
What's this construction??:
mask(c_ADEM_M) := g_ADEM(s_function_sel)(c_ADEM_M);
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;
Covered.
----------------------------
-- VME_IRQ_Controller.vhd --
----------------------------
- reset_n_i, we always name it rst_n_i
OK.
- 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.
'Tiny'.
----------------------------
-- simulation --
----------------------------
- general-cores submodule in wrong location. Move it from hdl/sim to
ip_cores/general-cores like we have for all the other repos.
Cf Maciej.
doc/review-2017-11-13/ML-vme64x-review.txt
View file @ 131c5766
......@@ -5,13 +5,13 @@ Note:
- Some changes suggested by me are committed to branch ml-vme64x_review
--------------------------------------------------------------------------------
General remarks (I started by trying to simulate the core, so some remarks
General remarks (I started by trying to simulate the core, so some remarks
about simulatin will come first):
0.1. In principle, if simulation is provided in a project, our (non-written)
convention to get the simulation running (without reading any info) would
convention to get the simulation running (without reading any info) would
be to do the following:
git clone
git submodule init
git submodule update
......@@ -19,85 +19,98 @@ about simulatin will come first):
hdlmake
make
do run.do
this is not the case and there is no README that is helpful in discovering
how to make the simulation. Moreover, the existing README in the main
folder is out-of-date.
I see several improvements that are spread among several points below.
0.2. I would recommend to follow our folder convention (which used to be on
ohwr.org and which is now gone, probably due to migration, so I copy+paste
it at the end of this file). Using this convention would require the
OK: let's follow this convention.
0.2. I would recommend to follow our folder convention (which used to be on
ohwr.org and which is now gone, probably due to migration, so I copy+paste
it at the end of this file). Using this convention would require the
following changes:
a) change /documents to /docs
a) change /documents to /doc
Done.
b) change hdl/sim to hdl/testbench
c) possibly movig hdl/sim/vme64x_bfm to hdl/sim/vme64x_bfm if this is a
OK.
c) keep vme64x_bfm in hdl/sim/vme64x_bfm if this is a
model of the vme64x (I do not know because it is undocumented)
OK.
0.3. the arrangement of simple_tb is confusing: first I thought that the folder
"modelsim" contains some modelsim-specific stuff. Again, how should I know
if there is no description/documentation/README. I'd recommend to do one
of the following:
a) move content from hdl/sim/simple_tb/modelsim/ to hdl/sim/simple_tb
a) move content from hdl/sim/simple_tb/modelsim/ to hdl/sim/simple_tb
(preferred if there are no plans to add simulation for other tools)
b) add README explaining this structure
Add a README.
0.4. hdl/sim/simple_tb/modelsim/run_all.sh is not working [committed fix]
OK.
0.5. It would be extremely useful to have some waveforms when running simulation.
I recommend doing one of the following:
a) modify run_all.sh to show the run simulations in waveform
b) add run.do script that runs one of the testbench cases [committed]
It is very helpful to the developer to push a wave.do with most important
signals. Ideally, the documentation provided to the user would match the
It is very helpful to the developer to push a wave.do with most important
signals. Ideally, the documentation provided to the user would match the
waveform so that when a developer runs the simulation, he can easily map
the output to the documentation.
OK with b).
0.6. Add general-cores as submodule in /hdl/ip_cores [committed]
even if vme64x is never used standalone and general-cores are used in this
project only when running simulation, I would still include it as submodule
in ip_cores for a number of reasons:
a) when someone takes the core to us in his/her project, it is useful to
know which ip_cores and which version he/she needs
b) to make the simulation easier to to run
a) when someone takes the core to us in his/her project, it is useful to
know which ip_cores and which version he/she needs
b) to make the simulation easier to to run
c) it seems to be a general policy
OK.
0.7. README in the main directory is completely out of date !
OK.
0.8. /documention folder:
- the notes from the 2012 review should stay,
- a new folder for the current review should be added.
- the notes from the 2012 review should stay,
Still in the repo.
- a new folder for the current review should be added.
Done.
- I would guess that some of the removed documents will be replaced with
updated versions. If this is not the case, it seems to me that the
currently provided documentation (the one reminding in the folder) is
NOT sufficient. At least, the vme64x_user_manual should be revived.
Moreover, the documentation in the project files seems to be out-of-date.
updated versions. If this is not the case, it seems to me that the
currently provided documentation (the one reminding in the folder) is
NOT sufficient. At least, the vme64x_user_manual should be revived.
Moreover, the documentation in the project files seems to be out-of-date.
OK.
0.9. consider refactoring as follows:
(see https://www.ohwr.org/attachments/554/VHDLcoding.pdf)
- the convention of prefix s_ for signals is generally avoided, remove s_
- the convention of using the suffix _a for asynchronous (input) signals is
useful and used, add
0.10. packages:
- I do not understand why you need two separate packages for the
- I do not understand why you need two separate packages for the
VME64xCore_Top and its wrapper xvme64x_core. I would recommend to
merge vme64x_pack.vhd and xvme64x_core_pkg.vhd into one file/pacakge
- a "private" package seems like a good idea and something that is used in
other projects. I would recommend to add vme64x_core_priv_pkg.vhd. Thanks
- a "private" package seems like a good idea and something that is used in
other projects. I would recommend to add vme64x_core_priv_pkg.vhd. Thanks
to this, you will be able to "use" this package and import the modules
from it, rather than having to call directly work.module
(as suggested in a latter comment).
0.11. the naming of the files in hdl/rtl is very chaotic:
- Ideally, and by the convention used in other projects, the name of a
- Ideally, and by the convention used in other projects, the name of a
wrapper differs from the name of the module it wrapps only by the suffix
"x", e.g.: wr_core.vhd and xwr_core.vhd, wrsw_rtu.vhd and xwrsw_rtu.vhd.
I'd recommend to follow this convention
- the capitalization and using (or not using) of "_" is very inconsistent,
please fix it.
0.12. In the description of all files, there exists section "dependencies", It
does not seem to be appropriately filled in. Remove it from all files or
0.12. In the description of all files, there exists section "dependencies", It
does not seem to be appropriately filled in. Remove it from all files or
fill in appropriately.I recommend the former.
0.13. git blame shows that there is not much left from the original code of
Pablo and Davide, add the other contributors as authors to appropriate
files (possibly all)
0.14. The descriptions in the files (which is currently the only documentation)
is written in poorly English that is hard to understand. It seems to be
out-of-date. It should be thoroughly reviewed. In comments to particular
files I point out unclear text. I also included some corrections in
out-of-date. It should be thoroughly reviewed. In comments to particular
files I point out unclear text. I also included some corrections in
commits
--------------------------------------------------------------------------------
xvme64x_core_pkg.vhd and vme64x_pack.vhd:
--------------------------------------------------------------------------------
......@@ -106,8 +119,8 @@ xvme64x_core_pkg.vhd and vme64x_pack.vhd:
--------------------------------------------------------------------------------
VME64xCore_Top.vhd
--------------------------------------------------------------------------------
2.1 [L:14-78]the description is written badly. I improved fragments in which I
knew what author wanted to say. The fragments that need to be explained
2.1 [L:14-78]the description is written badly. I improved fragments in which I
knew what author wanted to say. The fragments that need to be explained
better are listed below:
a) input or output
-- The Input signals from the WB bus aren't registered indeed the WB is a
......@@ -123,25 +136,25 @@ VME64xCore_Top.vhd
-- solution is better than the solution with FIFO.
-- In this base version of the core the FIFO is not implemented indeed the 2e
-- access modes aren't supported yet.
c) could you elaborate on this:
-- To access the CR/CSR space: AM = 0x2f --> this is A24 addressing type,
-- SINGLE transfer type. Base Address = Slot Number.
2.2 [L:101]the document says "-- last changes: see log." -> what log??? you mean
git? (it is similar in many files)
2.3 [L:367]use new private library for internal components, do not refer directly
2.3 [L:367]use new private library for internal components, do not refer directly
to "work.VME_Funct_Match" etc
2.4 [L:367-397] There is a dedicated module that is used in many projects for
2.4 [L:367-397] There is a dedicated module that is used in many projects for
synchronizing input asynchronous signals. It seems to include the same
functionality as the module used here gc_sync_register. My suggestion is
to use the same module as the other projects and think about removing
gc_sync_register from general cores.
--------------------------------------------------------------------------------
VME_bus.vhd
VME_bus.vhd
--------------------------------------------------------------------------------
3.1 Again, the description is poorly written, i.e.:
a) in the description below [L: 34-35], do you mean the "MAIN FSM"
a) in the description below [L: 34-35], do you mean the "MAIN FSM"
component? you should add in the text a name corresponding to the drawing:
-- The Access decode component decodes the address to check if the board is
-- the responding Slave. This component is of fundamental importance, indeed
......@@ -153,17 +166,17 @@ VME_bus.vhd
c) I do not understand this:
-- [...]; only one FPGA at time can
-- carry the vme64x core or other similar VME slave core.
3.2 the description of behavior concerning VME_AS_n_i is inconsistent with
the code [L:304 and 355]. In particular, the description says about
3.2 the description of behavior concerning VME_AS_n_i is inconsistent with
the code [L:304 and 355]. In particular, the description says about
acting upon rising/falling edge, while the condition is not detecting edge.
It seems a particularly important difference in L:355 where the FSM will be
re-entered when VME_AS_n_i is kept LOW all the time. I do not know which
behavior is correct (possibly, the description is wrong and the
behavior is correct (possibly, the description is wrong and the
implementation OK)
3.3 [L:497] the input signal VME_DS_n_i is already synchronized with the
3.3 [L:497] the input signal VME_DS_n_i is already synchronized with the
clock domain. There should be no metastability problem. And if there was
metastability problem, such a metastability would affect also VME_IACKIN_n_i
and VME_IACK_n_i that go through the same synchronization chain and are
and VME_IACK_n_i that go through the same synchronization chain and are
used directly in the FSM. I would remove the comment.
--------------------------------------------------------------------------------
......@@ -171,7 +184,7 @@ VME_Funct_Match:
--------------------------------------------------------------------------------
4.1 [L: 13] missing description, at least one sentence, please :)
4.2 [L:94-114]: the code optimization in this process deteriorates code
readability. I would recommend to rewrite the code a bit to make it clear
readability. I would recommend to rewrite the code a bit to make it clear
what happens,i.e.:
process (clk_i)
begin
......@@ -194,7 +207,7 @@ VME_Funct_Match:
end process;
--------------------------------------------------------------------------------
VME_IRQ_Controller.vhd
VME_IRQ_Controller.vhd
--------------------------------------------------------------------------------
5.1 The description is not only poorly written but also misleading. I cannot find
in the VHDL code the FSM that is described, i.e.:
......@@ -215,7 +228,7 @@ VME_IRQ_Controller.vhd
-- The interrupter wait the IACKIN falling edge in the IRQ state, so if the
-- interrupter don't have interrupt pending for sure it will not respond
-- because it is in IDLE.
5.2 [L:156-158] I do not understand the second part of this sentence
5.2 [L:156-158] I do not understand the second part of this sentence
-- Interrupts are automatically masked for g_RETRY_TIMEOUT (i.e. 1 ms) once
-- they are acknowledge by the interrupt handler until they are deasserted
-- by the interrupter.
......@@ -223,10 +236,10 @@ VME_IRQ_Controller.vhd
5.4 [L:190] either remove "fsm" from the process name, or make it a real FSM.
--------------------------------------------------------------------------------
VME_User_CSR.vhd
VME_User_CSR.vhd
--------------------------------------------------------------------------------
6.1 The information in this description should be also in the top module
and/or in any other documentation that will be created. A developer who
and/or in any other documentation that will be created. A developer who
takes a top module should not be forced to go here to read this info
......@@ -239,7 +252,7 @@ MyProj
|-circuit_board
|-doc
|-hdl
| |-ip_cores
| |-ip_cores
| | |-wr-cores (submodule)
| | |-general-cores (submodule)
| |
......@@ -263,7 +276,7 @@ MyProj
| | |-myStuff
| | |-Manifest.py
| | |-top_z.xise
| |
| |
| |
| |-testbench
| | |-include
......@@ -288,6 +301,6 @@ MyProj
| |-embedded
| |-host
|-mechanics
reference:
http://www.ohwr.org/projects/fmc-delay-1ns-8cha/repository
\ No newline at end of file
http://www.ohwr.org/projects/fmc-delay-1ns-8cha/repository
doc/review-2017-11-13/dlamprid-vme64x_review.org
View file @ 131c5766
* General notes
- pull dlamprid-vme64x_review for both the core and the SVEC demo (vmecore_test)
+ adjust/merge as you see fit
Done
- [Update]/[Delete] top-level README.txt?
Done
- [Update] HDL headers
OK.
- [Update]/[Delete]/[Move to user manual] header comments with technical explanations?
OK.
- Good opportunity to rename files, entities and instances. eg:
+ pack or pkg?
_pkg (same name as the design unit name).
+ capital or small case file names?
The same case, lower case.
+ ...
- does not strictly follow proposed folder structure
See Maciej comment
* VME64x core
- get rid of SVEC_ID, CERN_ID etc from package and default generic values
OK for default generic
- 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
OK.
- shal we name/label all processes?
NO, but...
** 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)
Ok.
- gc_sync_register: does it even make sense? Slide #95 of NASA radiation tolerant design presentation:
https://indico.cern.ch/event/663761/contributions/2710422/attachments/1537821/2410163/Berg_SynchronousDesign_2017.pdf
No problem, discussion about width postponed.
*** VME_CR_CSR_Space.vhd
- delete unused port vme_sysfail_ena_o?
OK.
* Simulation
- how am I supposed to run it? A README would help
- why open the GUI at the end of make?
OK.
- why keep around VME BFM if you are not using it? Why the SVEC-specific file in the VME BFM?
Removed. To be restored.
- why not (also) GHDL if Modelsim is not necessary?
Shouldn't be hard, excercise let to the reader.
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